| Message ID | 20230117104508.GB12547@altlinux.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wifi: brcmfmac: Fix allocation size | expand |
"Alexey V. Vissarionov" <gremlin@altlinux.org> writes: > The "pkt" is a pointer to struct sk_buff, so it's just 4 or 8 > bytes, while the structure itself is much bigger. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: bbd1f932e7c45ef1 ("brcmfmac: cleanup ampdu-rx host reorder code") > Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org> > > diff --git > a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > index 36af81975855c525..0d283456da331464 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > @@ -1711,7 +1711,7 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, > struct sk_buff *pkt) > buf_size = sizeof(*rfi); > max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET]; > > - buf_size += (max_idx + 1) * sizeof(pkt); > + buf_size += (max_idx + 1) * sizeof(struct sk_buff); Wouldn't sizeof(*pkt) be better? Just like with sizeof(*rfi) few lines above.
On Tue, Jan 17, 2023 at 01:45:08PM +0300, Alexey V. Vissarionov wrote: > The "pkt" is a pointer to struct sk_buff, so it's just 4 or 8 > bytes, while the structure itself is much bigger. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: bbd1f932e7c45ef1 ("brcmfmac: cleanup ampdu-rx host reorder code") > Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org> > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > index 36af81975855c525..0d283456da331464 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c > @@ -1711,7 +1711,7 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt) > buf_size = sizeof(*rfi); > max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET]; > > - buf_size += (max_idx + 1) * sizeof(pkt); > + buf_size += (max_idx + 1) * sizeof(struct sk_buff); > > /* allocate space for flow reorder info */ > brcmf_dbg(INFO, "flow-%d: start, maxidx %d\n", Hi Alexey, This is followed by: rfi = kzalloc(buf_size, GFP_ATOMIC); ... rfi->pktslots = (struct sk_buff **)(rfi + 1); The type of rfi is struct brcmf_ampdu_rx_reorder, which looks like this: struct brcmf_ampdu_rx_reorder { struct sk_buff **pktslots; ... }; And it looks to me that pkt is used as an array of (struct sk_buff *). So in all, it seems to me that the current code is correct. Is there a particular code that leads you to think otherwise? Kind regards, Simon
On 2023-01-17 13:05:24 +0200, Kalle Valo wrote: >> - buf_size += (max_idx + 1) * sizeof(pkt); >> + buf_size += (max_idx + 1) * sizeof(struct sk_buff); > Wouldn't sizeof(*pkt) be better? Usually sizeof(type) produces less errors than sizeof(var)... > Just like with sizeof(*rfi) few lines above. ... but to keep consistency sizeof(*pkt) would also be ok.
On 2023-01-17 12:13:06 +0100, Simon Horman wrote: >> buf_size = sizeof(*rfi); >> max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET]; >> - buf_size += (max_idx + 1) * sizeof(pkt); >> + buf_size += (max_idx + 1) * sizeof(struct sk_buff); > This is followed by: > rfi = kzalloc(buf_size, GFP_ATOMIC); > ... > rfi->pktslots = (struct sk_buff **)(rfi + 1); > The type of rfi is struct brcmf_ampdu_rx_reorder, which > looks like this: > struct brcmf_ampdu_rx_reorder > { struct sk_buff **pktslots; ... }; > And it looks to me that pkt is used as an array of > (struct sk_buff *). > So in all, it seems to me that the current code is correct. So, the buf_size is a sum of sizeof(struct brcmf_ampdu_rx_reorder) and size of array of pointers... and yes, this array is filled with pointers: rfi->pktslots[rfi->cur_idx] = pkt; Hmmm... looks correct. Sorry for bothering.
On 1/17/2023 12:54 PM, Alexey V. Vissarionov wrote: > On 2023-01-17 12:13:06 +0100, Simon Horman wrote: > > >> buf_size = sizeof(*rfi); > >> max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET]; > >> - buf_size += (max_idx + 1) * sizeof(pkt); > >> + buf_size += (max_idx + 1) * sizeof(struct sk_buff); > > > This is followed by: > > rfi = kzalloc(buf_size, GFP_ATOMIC); > > ... > > rfi->pktslots = (struct sk_buff **)(rfi + 1); > > The type of rfi is struct brcmf_ampdu_rx_reorder, which > > looks like this: > > struct brcmf_ampdu_rx_reorder > > { struct sk_buff **pktslots; ... }; > > And it looks to me that pkt is used as an array of > > (struct sk_buff *). > > So in all, it seems to me that the current code is correct. > > So, the buf_size is a sum of sizeof(struct brcmf_ampdu_rx_reorder) > and size of array of pointers... and yes, this array is filled with > pointers: rfi->pktslots[rfi->cur_idx] = pkt; > > Hmmm... looks correct. Sorry for bothering. No problem. Nice to see the water went still without me chiming in. It has been a while since this was added to the driver and there could be issues with this code, but if this allocation was wrong we would have had reports by now. Thanks, Arend
"Alexey V. Vissarionov" <gremlin@altlinux.org> writes: > On 2023-01-17 13:05:24 +0200, Kalle Valo wrote: > > >> - buf_size += (max_idx + 1) * sizeof(pkt); > >> + buf_size += (max_idx + 1) * sizeof(struct sk_buff); > > Wouldn't sizeof(*pkt) be better? > > Usually sizeof(type) produces less errors than sizeof(var)... This matter of taste really but FWIW I prefer sizeof(var) as then the type can't be different by accident. And the coding style says something similar, although that's related to memory allocation: https://www.kernel.org/doc/html/latest/process/coding-style.html#allocating-memory
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c index 36af81975855c525..0d283456da331464 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c @@ -1711,7 +1711,7 @@ void brcmf_fws_rxreorder(struct brcmf_if *ifp, struct sk_buff *pkt) buf_size = sizeof(*rfi); max_idx = reorder_data[BRCMF_RXREORDER_MAXIDX_OFFSET]; - buf_size += (max_idx + 1) * sizeof(pkt); + buf_size += (max_idx + 1) * sizeof(struct sk_buff); /* allocate space for flow reorder info */ brcmf_dbg(INFO, "flow-%d: start, maxidx %d\n",
The "pkt" is a pointer to struct sk_buff, so it's just 4 or 8 bytes, while the structure itself is much bigger. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: bbd1f932e7c45ef1 ("brcmfmac: cleanup ampdu-rx host reorder code") Signed-off-by: Alexey V. Vissarionov <gremlin@altlinux.org>