Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response
From: Johannes Berg
Date: Wed Apr 22 2026 - 15:16:11 EST
On Wed, 2026-04-22 at 11:26 -0700, Brian Norris wrote:
>
> > + u16 resp_size = le16_to_cpu(resp->size);
> > + u16 count = le16_to_cpu(sta_list->sta_count);
> > + u16 max_count;
> >
> > - for (i = 0; i < (le16_to_cpu(sta_list->sta_count)); i++) {
> > + if (resp_size < sizeof(*resp) - sizeof(resp->params) + sizeof(*sta_list))
> > + return -EINVAL;
> > + max_count = (resp_size - sizeof(*resp) + sizeof(resp->params) -
> > + sizeof(*sta_list)) / sizeof(*sta_info);
>
> The repeated arithmetic is a bit weird, but I'm not sure if it'd
> actually be better to stash it in its own variable. Seems good enough I
> suppose.
I think it might be simpler if instead trying to limit:
> > + count = min(count, max_count);
it'd just check the needed length based on the given count, and reject
anything above that?
Also, the whole sizeof(*resp) - sizeof(resp->params) etc. shouldn't be
there, that should just be
offsetofend(resp, sta_list.tlv)
and then suddenly it becomes _way_ simpler:
if (resp_size < offsetofend(resp, sta_list.tlv))
return -EINVAL;
if (resp_size < offsetofend(resp, sta_list.tlv) +
sizeof(*sta_info) * le16_to_cpu(sta_.list->sta_count))
return -EINVAL;
But regardless, I question the sanity of checking the size against the
size the firmware said the whole thing was going to be, rather than
checking against the actual buffer size ...
johannes