Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()
From: Arend van Spriel
Date: Mon Aug 03 2026 - 04:58:49 EST
On 03/08/2026 10:35, Johannes Berg wrote:
On Mon, 2026-08-03 at 10:21 +0200, Arend van Spriel wrote:True. And yes, the existing vfree() should go, but I figured that goes without saying so I did not mention that. I am fine with your proposal, but I guess the author will have to submit a v2 patch, right?
I looked at moving the vfree() to the done: label, but the code also
gets there in the success path to wakeup waiters. In the success path
the buf is stored for further processing so it should not be freed.
Hah, my bad, sorry!
Maybe better to do:Actually it turns out brcmf_sdio_read_control() is only called once so
spin_unlock_bh(&bus->rxctl_lock);
brcmf_sdio_dcmd_resp_wake(bus);
return;
fail:
vfree(buf);
/* Awake any waiters */
brcmf_sdio_dcmd_resp_wake(bus);
}
the brcmf_sdio_dcmd_resp_wake() can be taken outside the function.
Or just something like this?
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -1827,15 +1827,17 @@ brcmf_sdio_read_control(struct brcmf_sdio *bus, u8 *hdr, uint len, uint doff)
if (bus->rxctl) {
brcmf_err("last control frame is being processed.\n");
spin_unlock_bh(&bus->rxctl_lock);
- vfree(buf);
goto done;
}
bus->rxctl = buf + doff;
bus->rxctl_orig = buf;
bus->rxlen = len - doff;
spin_unlock_bh(&bus->rxctl_lock);
+ /* buffer is queued */
+ buf = NULL;
done:
+ vfree(buf);
/* Awake any waiters */
brcmf_sdio_dcmd_resp_wake(bus);
}
It's already using vmalloc() so seems it wouldn't care that much about
calling vfree(NULL) on the success path?
Gr. AvS