| Message ID | 20241107132903.13513-1-nvbolhuis@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 857282b819cbaa0675aaab1e7542e2c0579f52d7 |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] wifi: brcmfmac: Fix oops due to NULL pointer dereference in 'brcmf_sdiod_sglist_rw' | expand |
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.
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.
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.
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
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
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()
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);