[PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[nvbolhuis]

From: Norbert van Bolhuis <nvbolhuis@gmail.com>

This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of queued SKBs
are sent from the pkt queue.

The problem is the number of entries in the pre-allocated sgtable, it is
nents = max(rxglom_size, txglom_size) + max(rxglom_size, txglom_size) >> 4 + 1.
Given the default [rt]xglom_size=32 it's actually 35 which is too small.
Worst case, the pkt queue can end up with 64 SKBs. This occurs when a new SKB
is added for each original SKB if tailroom isn't enough to hold tail_pad.
At least one sg entry is needed for each SKB. So, eventually the "skb_queue_walk loop"
in brcmf_sdiod_sglist_rw may run out of sg entries. This makes sg_next return
NULL and this causes the oops.

The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able handle
the worst-case.
Btw. this requires only 64-35=29 * 16 (or 20 if CONFIG_NEED_SG_DMA_LENGTH) = 464
additional bytes of memory.

Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 17f6b33beabd..42d991d9f8cb 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -770,7 +770,7 @@ void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev)
 
 	nents = max_t(uint, BRCMF_DEFAULT_RXGLOM_SIZE,
 		      sdiodev->settings->bus.sdio.txglomsz);
-	nents += (nents >> 4) + 1;
+	nents *= 2;
 
 	WARN_ON(nents > sdiodev->max_segment_count);
 
-- 
2.42.0

Re: [PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[Kalle Valo]

nvbolhuis@gmail.com writes:

> From: Norbert van Bolhuis <nvbolhuis@gmail.com>
>
> This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
> when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of queued SKBs
> are sent from the pkt queue.
>
> The problem is the number of entries in the pre-allocated sgtable, it is
> nents = max(rxglom_size, txglom_size) + max(rxglom_size, txglom_size) >> 4 + 1.
> Given the default [rt]xglom_size=32 it's actually 35 which is too small.
> Worst case, the pkt queue can end up with 64 SKBs. This occurs when a new SKB
> is added for each original SKB if tailroom isn't enough to hold tail_pad.
> At least one sg entry is needed for each SKB. So, eventually the "skb_queue_walk loop"
> in brcmf_sdiod_sglist_rw may run out of sg entries. This makes sg_next return
> NULL and this causes the oops.
>
> The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able handle
> the worst-case.
> Btw. this requires only 64-35=29 * 16 (or 20 if CONFIG_NEED_SG_DMA_LENGTH) = 464
> additional bytes of memory.
>
> Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>

What changed from v1? Please include a list of changes after '--' line,
but no need to resend because of this.

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

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

Re: [PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[N van Bolhuis]

Op do 7 nov 2024 om 15:14 schreef Kalle Valo <kvalo@kernel.org>:
>
> nvbolhuis@gmail.com writes:
>
> > From: Norbert van Bolhuis <nvbolhuis@gmail.com>
> >
> > This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
> > when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of queued SKBs
> > are sent from the pkt queue.
> >
> > The problem is the number of entries in the pre-allocated sgtable, it is
> > nents = max(rxglom_size, txglom_size) + max(rxglom_size, txglom_size) >> 4 + 1.
> > Given the default [rt]xglom_size=32 it's actually 35 which is too small.
> > Worst case, the pkt queue can end up with 64 SKBs. This occurs when a new SKB
> > is added for each original SKB if tailroom isn't enough to hold tail_pad.
> > At least one sg entry is needed for each SKB. So, eventually the "skb_queue_walk loop"
> > in brcmf_sdiod_sglist_rw may run out of sg entries. This makes sg_next return
> > NULL and this causes the oops.
> >
> > The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able handle
> > the worst-case.
> > Btw. this requires only 64-35=29 * 16 (or 20 if CONFIG_NEED_SG_DMA_LENGTH) = 464
> > additional bytes of memory.
> >
> > Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>
>
> What changed from v1? Please include a list of changes after '--' line,
> but no need to resend because of this.
>

Nothing changed, I just added the s-o-b.

Re: [PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[Kalle Valo]

N van Bolhuis <nvbolhuis@gmail.com> writes:

> Op do 7 nov 2024 om 15:14 schreef Kalle Valo <kvalo@kernel.org>:
>
>>
>> nvbolhuis@gmail.com writes:
>>
>> > From: Norbert van Bolhuis <nvbolhuis@gmail.com>
>> >
>> > This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
>> > when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of queued SKBs
>> > are sent from the pkt queue.
>> >
>> > The problem is the number of entries in the pre-allocated sgtable, it is
>> > nents = max(rxglom_size, txglom_size) + max(rxglom_size, txglom_size) >> 4 + 1.
>> > Given the default [rt]xglom_size=32 it's actually 35 which is too small.
>> > Worst case, the pkt queue can end up with 64 SKBs. This occurs when a new SKB
>> > is added for each original SKB if tailroom isn't enough to hold tail_pad.
>> > At least one sg entry is needed for each SKB. So, eventually the "skb_queue_walk loop"
>> > in brcmf_sdiod_sglist_rw may run out of sg entries. This makes sg_next return
>> > NULL and this causes the oops.
>> >
>> > The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able handle
>> > the worst-case.
>> > Btw. this requires only 64-35=29 * 16 (or 20 if CONFIG_NEED_SG_DMA_LENGTH) = 464
>> > additional bytes of memory.
>> >
>> > Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>
>>
>> What changed from v1? Please include a list of changes after '--' line,
>> but no need to resend because of this.
>>
>
> Nothing changed, I just added the s-o-b.

That's still something you should mention in the changelog, but this
mail is good enough this time.

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

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

Re: [PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[Arend van Spriel]

On 11/7/2024 5:09 PM, N van Bolhuis wrote:
> Op do 7 nov 2024 om 15:14 schreef Kalle Valo <kvalo@kernel.org>:
>>
>> nvbolhuis@gmail.com writes:
>>
>>> From: Norbert van Bolhuis <nvbolhuis@gmail.com>
>>>
>>> This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
>>> when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of queued SKBs
>>> are sent from the pkt queue.
>>>
>>> The problem is the number of entries in the pre-allocated sgtable, it is
>>> nents = max(rxglom_size, txglom_size) + max(rxglom_size, txglom_size) >> 4 + 1.
>>> Given the default [rt]xglom_size=32 it's actually 35 which is too small.
>>> Worst case, the pkt queue can end up with 64 SKBs. This occurs when a new SKB
>>> is added for each original SKB if tailroom isn't enough to hold tail_pad.
>>> At least one sg entry is needed for each SKB. So, eventually the "skb_queue_walk loop"
>>> in brcmf_sdiod_sglist_rw may run out of sg entries. This makes sg_next return
>>> NULL and this causes the oops.
>>>
>>> The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able handle
>>> the worst-case.
>>> Btw. this requires only 64-35=29 * 16 (or 20 if CONFIG_NEED_SG_DMA_LENGTH) = 464
>>> additional bytes of memory.
>>>
>>> Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>
>>
>> What changed from v1? Please include a list of changes after '--' line,
>> but no need to resend because of this.
>>
> 
> Nothing changed, I just added the s-o-b.

Hoi Norbert,

Welkom in de wondere wereld van linux kernel development. De proces 
beschrijving van Kalle is een goede referentie. Go with the flow.

Jouw naam klonk mij bekend in de oren al is de Lucent tijd al ver achter 
ons. Mijn kamergenoot hier op kantoor, Hante Meuleman, kon zich jou ook 
herinneren, maar dat is dan ook minder lang geleden.

Groeten,
Arend

Re: [PATCH v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw'

[Arend van Spriel]

On 11/7/2024 8:31 PM, Arend van Spriel wrote:
> On 11/7/2024 5:09 PM, N van Bolhuis wrote:
>> Op do 7 nov 2024 om 15:14 schreef Kalle Valo <kvalo@kernel.org>:
>>>
>>> nvbolhuis@gmail.com writes:
>>>
>>>> From: Norbert van Bolhuis <nvbolhuis@gmail.com>
>>>>
>>>> This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
>>>> when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of 
>>>> queued SKBs
>>>> are sent from the pkt queue.
>>>>
>>>> The problem is the number of entries in the pre-allocated sgtable, 
>>>> it is
>>>> nents = max(rxglom_size, txglom_size) + max(rxglom_size, 
>>>> txglom_size) >> 4 + 1.
>>>> Given the default [rt]xglom_size=32 it's actually 35 which is too 
>>>> small.
>>>> Worst case, the pkt queue can end up with 64 SKBs. This occurs when 
>>>> a new SKB
>>>> is added for each original SKB if tailroom isn't enough to hold 
>>>> tail_pad.
>>>> At least one sg entry is needed for each SKB. So, eventually the 
>>>> "skb_queue_walk loop"
>>>> in brcmf_sdiod_sglist_rw may run out of sg entries. This makes 
>>>> sg_next return
>>>> NULL and this causes the oops.
>>>>
>>>> The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able 
>>>> handle
>>>> the worst-case.
>>>> Btw. this requires only 64-35=29 * 16 (or 20 if 
>>>> CONFIG_NEED_SG_DMA_LENGTH) = 464
>>>> additional bytes of memory.
>>>>
>>>> Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>
>>>
>>> What changed from v1? Please include a list of changes after '--' line,
>>> but no need to resend because of this.
>>>
>>
>> Nothing changed, I just added the s-o-b.
> 
> Hoi Norbert,
> 
> Welkom in de wondere wereld van linux kernel development. De proces 
> beschrijving van Kalle is een goede referentie. Go with the flow.
> 
> Jouw naam klonk mij bekend in de oren al is de Lucent tijd al ver achter 
> ons. Mijn kamergenoot hier op kantoor, Hante Meuleman, kon zich jou ook 
> herinneren, maar dat is dan ook minder lang geleden.

Yikes. Sorry for the (dutch) spam. Blindly hit the 'reply to all' button.

Regards,
Arend

Re: [v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in brcmf_sdiod_sglist_rw()

[Kalle Valo]

nvbolhuis@gmail.com wrote:

> From: Norbert van Bolhuis <nvbolhuis@gmail.com>
> 
> This patch fixes a NULL pointer dereference bug in brcmfmac that occurs
> when a high 'sd_sgentry_align' value applies (e.g. 512) and a lot of queued SKBs
> are sent from the pkt queue.
> 
> The problem is the number of entries in the pre-allocated sgtable, it is
> nents = max(rxglom_size, txglom_size) + max(rxglom_size, txglom_size) >> 4 + 1.
> Given the default [rt]xglom_size=32 it's actually 35 which is too small.
> Worst case, the pkt queue can end up with 64 SKBs. This occurs when a new SKB
> is added for each original SKB if tailroom isn't enough to hold tail_pad.
> At least one sg entry is needed for each SKB. So, eventually the "skb_queue_walk loop"
> in brcmf_sdiod_sglist_rw may run out of sg entries. This makes sg_next return
> NULL and this causes the oops.
> 
> The patch sets nents to max(rxglom_size, txglom_size) * 2 to be able handle
> the worst-case.
> Btw. this requires only 64-35=29 * 16 (or 20 if CONFIG_NEED_SG_DMA_LENGTH) = 464
> additional bytes of memory.
> 
> Signed-off-by: Norbert van Bolhuis <nvbolhuis@gmail.com>

Patch applied to wireless-next.git, thanks.

857282b819cb wifi: brcmfmac: Fix oops due to NULL pointer dereference in brcmf_sdiod_sglist_rw()

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20241107132903.13513-1-nvbolhuis@gmail.com/

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