Re: [PATCH v3 3/6] wifi: mwifiex: fix OOB read from firmware sta_count in station list response

[Brian Norris <briannorris@chromium.org>]

On Wed, Apr 22, 2026 at 09:12:11PM +0200, Johannes Berg wrote:
> 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?

That might be better.

> Also, the whole sizeof(*resp) - sizeof(resp->params) etc. shouldn't be
> there, that should just be
> 
> 	offsetofend(resp, sta_list.tlv)

TIL. I don't recall seeing that macro before. Or at least, I didn't know
it well enough to recommend it.

> 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;

Looks good to me.

> 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 ...

Admittedly, I get lost in this driver sometimes...
...but I think you have a very good point. AFAICT, we never do anything
to check the size of adapter->curr_cmd->resp_skb. We generally assume
it's big enough to fit 'struct host_cmd_ds_command' (since we allocate
it ourselves). But we don't ever go back to check these
dynamically-sized fields don't overflow it.

Brian