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 <[email protected]>
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",
--
Alexey V. Vissarionov
gremlin ??? altlinux ??? org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
"Alexey V. Vissarionov" <[email protected]> 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 <[email protected]>
>
> 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.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
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 <[email protected]>
>
> 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.
--
Alexey V. Vissarionov
gremlin ??? altlinux ??? org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
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.
--
Alexey V. Vissarionov
gremlin ??? altlinux ??? org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net
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" <[email protected]> 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
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches