[PATCH] wifi: brcmfmac: drop glom superframe on header parse failure

[PATCH] wifi: brcmfmac: drop glom superframe on header parse failure

[Shirong Zhao]

brcmf_sdio_rxglom() uses rd_new.len_nxtfrm and rd_new.dat_offset
right after the superframe brcmf_sdio_hdparse() without checking
errcode.  On parse failure those fields are either uninitialized
(the early -ENODATA/-EIO/-EPROTO returns) or hold a rejected value
(the -ENXIO bad-offset return), so bus->cur_read.len gets garbage
and skb_pull() runs with a garbage length.

Check errcode first and drop the superframe with the same cleanup
used for a bad subframe below.

Verified against wireless.git main (9727d1c4e1c9), the current
wireless fixes tree.

Signed-off-by: Shirong Zhao <shxzhaosr@163.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 381801af3..53ffb8dba 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -1649,6 +1649,18 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
 		errcode = brcmf_sdio_hdparse(bus, pfirst->data, &rd_new,
 					     BRCMF_SDIO_FT_SUPER);
 		sdio_release_host(bus->sdiodev->func1);
+		if (errcode) {
+			/* Superframe header is bad; rd_new fields are not
+			 * valid here. Drop it like a bad subframe below.
+			 */
+			sdio_claim_host(bus->sdiodev->func1);
+			brcmf_sdio_rxfail(bus, true, false);
+			bus->sdcnt.rxglomfail++;
+			brcmf_sdio_free_glom(bus);
+			sdio_release_host(bus->sdiodev->func1);
+			bus->cur_read.len = 0;
+			return 0;
+		}
 		bus->cur_read.len = rd_new.len_nxtfrm << 4;
 
 		/* Remove superframe header, remember offset */

Re: [PATCH] wifi: brcmfmac: drop glom superframe on header parse failure

[Arend van Spriel]

On 08/10/2026 12:33, Shirong Zhao wrote:
> brcmf_sdio_rxglom() uses rd_new.len_nxtfrm and rd_new.dat_offset
> right after the superframe brcmf_sdio_hdparse() without checking
> errcode.  On parse failure those fields are either uninitialized
> (the early -ENODATA/-EIO/-EPROTO returns) or hold a rejected value
> (the -ENXIO bad-offset return), so bus->cur_read.len gets garbage
> and skb_pull() runs with a garbage length.
> 
> Check errcode first and drop the superframe with the same cleanup
> used for a bad subframe below.
> 
> Verified against wireless.git main (9727d1c4e1c9), the current
> wireless fixes tree.

Thanks for the patch.
> Signed-off-by: Shirong Zhao <shxzhaosr@163.com>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 12 ++++++++++++
>   1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index 381801af3..53ffb8dba 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -1649,6 +1649,18 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
>   		errcode = brcmf_sdio_hdparse(bus, pfirst->data, &rd_new,
>   					     BRCMF_SDIO_FT_SUPER);

I would avoid the extra release/claim host below. I modified your change 
to what I think it should be:

> +		if (errcode) {
> +			/* Superframe header is bad; rd_new fields are not
> +			 * valid here. Drop it like a bad subframe below.
> +			 */
> +			brcmf_sdio_rxfail(bus, true, false);
> +			bus->sdcnt.rxglomfail++;
> +			brcmf_sdio_free_glom(bus);
> +			sdio_release_host(bus->sdiodev->func1);
> +			bus->cur_read.len = 0;
> +			return 0;
> +		}
>   		sdio_release_host(bus->sdiodev->func1);
>   		bus->cur_read.len = rd_new.len_nxtfrm << 4;
>   
>   		/* Remove superframe header, remember offset */

Regards,
Arend

[PATCH v2] wifi: brcmfmac: drop glom superframe on header parse failure

[Shirong Zhao]

brcmf_sdio_rxglom() uses rd_new.len_nxtfrm and rd_new.dat_offset
right after the superframe brcmf_sdio_hdparse() without checking
errcode.  On parse failure those fields are either uninitialized
(the early -ENODATA/-EIO/-EPROTO returns) or hold a rejected value
(the -ENXIO bad-offset return), so bus->cur_read.len gets garbage
and skb_pull() runs with a garbage length.

Check errcode first and drop the superframe with the same cleanup
used for a bad subframe below.

Verified against wireless.git main (fd4e7f1137291), the current
wireless fixes tree.

Signed-off-by: Shirong Zhao <shxzhaosr@163.com>
---
Changes in v2: check errcode before sdio_release_host() so the
cleanup runs under the already-claimed host, avoiding the extra
release/claim cycle (suggested by Arend van Spriel).
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 381801af3..abe8d44ec 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -1648,6 +1648,17 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
 		sdio_claim_host(bus->sdiodev->func1);
 		errcode = brcmf_sdio_hdparse(bus, pfirst->data, &rd_new,
 					     BRCMF_SDIO_FT_SUPER);
+		if (errcode) {
+			/* Superframe header is bad; rd_new fields are not
+			 * valid here. Drop it like a bad subframe below.
+			 */
+			brcmf_sdio_rxfail(bus, true, false);
+			bus->sdcnt.rxglomfail++;
+			brcmf_sdio_free_glom(bus);
+			sdio_release_host(bus->sdiodev->func1);
+			bus->cur_read.len = 0;
+			return 0;
+		}
 		sdio_release_host(bus->sdiodev->func1);
 		bus->cur_read.len = rd_new.len_nxtfrm << 4;
 
 		/* Remove superframe header, remember offset */