Re: {Disarmed} Two potential 1-byte BOF in cfg80211.c of driver MWIFIEX

Re: {Disarmed} Two potential 1-byte BOF in cfg80211.c of driver MWIFIEX

[Greg KH]

On Sun, Sep 15, 2024 at 06:12:21PM +0000, Sherlock Fang wrote:
> Dear Security Team,
> 
> 
> I have used static code analysis tools to scan the Linux Kernel of the latest build, and with collaborative help from Prof. Yueqi Chen, we have been able to manually confirm the presence of two 1-byte BOF in drivers/net/wireless/marvell/mwifiex/cfg80211.c
> 
> At both line https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/cfg80211.c#L2679 and https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/cfg80211.c#L2777, the calls to
> 
> memcpy(&priv->vs_ie[i].ie, ie, sizeof(*ie) + ie->len);
> 
> are theoretically possible to cause a buffer overflow since the destination buffer is an array of bytes with a size defined by MWIFIEX_MAX_VSIE_LEN<https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/ioctl.h#L416> of 256 bytes, which is the maximum amount of data that can safely be stored in priv->vs_ie[i].ie<https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/main.h#L483>. The size parameter "sizeof(*ie) + ie->len" theoretically has the maximum value of "2 + 255 = 257" given that len is of type u8 which has upper limit of 255, and sizeof(*ie) is equivalent to sizeof(struct ieee_types_header<https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/main.h#L343>), that is just the size of two u8 field (two bytes).
> 
> Our suggested fix would be to add a check before calling memcpy:
> 
> If (sizeof(*ie) + ie->len > sizeof(&priv->vs_ie[i].ie)) {
>       continue;
> }
> 
> memcpy(&priv->vs_ie[i].ie, ie, sizeof(*ie) + ie->len);

Great, can you submit a real patch for this so that it can be reviewed
and accepted if ok?

Also, you are sure that the data being copied could really be that big,
right?  Where does it come from?

thanks,

greg k-h

Re: {Disarmed} Two potential 1-byte BOF in cfg80211.c of driver MWIFIEX

[Johannes Berg]

Hi,

> > I have used static code analysis tools to scan the Linux Kernel of the latest build, and with collaborative help from Prof. Yueqi Chen, we have been able to manually confirm the presence of two 1-byte BOF in drivers/net/wireless/marvell/mwifiex/cfg80211.c

Not sure what "BOF" means in this context, I guess buffer overflow. Not
sure why 1 byte?

> > At both line https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/cfg80211.c#L2679 and https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/cfg80211.c#L2777, the calls to
> > 
> > memcpy(&priv->vs_ie[i].ie, ie, sizeof(*ie) + ie->len);

The line numbers are wrong now, but it seems this is in scan/sched scan
respectively.

> > are theoretically possible to cause a buffer overflow

No.

> > since the destination buffer is an array of bytes with a size defined by MWIFIEX_MAX_VSIE_LEN<https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/ioctl.h#L416> of 256 bytes, which is the maximum amount of data that can safely be stored in priv->vs_ie[i].ie<https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/main.h#L483>.

Indeed, the destination array is MWIFIEX_MAX_VSIE_LEN.

> > The size parameter "sizeof(*ie) + ie->len" theoretically has the maximum value of "2 + 255 = 257" given that len is of type u8 which has upper limit of 255, and sizeof(*ie) is equivalent to sizeof(struct ieee_types_header<https://github.com/torvalds/linux/blob/master/drivers/net/wireless/marvell/mwifiex/main.h#L343>), that is just the size of two u8 field (two bytes).

Yes and no. Your argumentation isn't completely _wrong_, in that
mathematically, indeed 2 + 255 == 257. Also, you're correct in saying
that the sizeof() results in 2, and the maximum value of a u8 can be
255.

However, the value of ie->len cannot actually be 255 in this context.

Not to say that it might not be better for the code to be more
immediately obviously correct, and there's always a chance that somebody
might break the properties here pretty easily, but a complete analysis
of the data used here would have shown you that it cannot indeed be a
security bug, and therefore there's no reason to waste everyone's time
by treating it as some kind of secret.

johannes