[PATCH] wifi: brcmfmac: Fix allocation size

[PATCH] wifi: brcmfmac: Fix allocation size

[Alexey V. Vissarionov]

[-- Attachment #1: Type: text/plain, Size: 1189 bytes --]

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",



-- 
Alexey V. Vissarionov
gremlin ПРИ altlinux ТЧК org; +vii-cmiii-ccxxix-lxxix-xlii
GPG: 0D92F19E1C0DC36E27F61A29CD17E2B43D879005 @ hkp://keys.gnupg.net

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] wifi: brcmfmac: Fix allocation size

[Kalle Valo]

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

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH] wifi: brcmfmac: Fix allocation size

[Simon Horman]

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

Re: [PATCH] wifi: brcmfmac: Fix allocation size

[Alexey V. Vissarionov]

[-- Attachment #1: Type: text/plain, Size: 535 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] wifi: brcmfmac: Fix allocation size

[Alexey V. Vissarionov]

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

Re: [PATCH] wifi: brcmfmac: Fix allocation size

[Arend van Spriel]

[-- Attachment #1: Type: text/plain, Size: 1248 bytes --]

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

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]

Re: [PATCH] wifi: brcmfmac: Fix allocation size

[Kalle Valo]

"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

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches