[PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Abdun Nihaal]

The memory allocated for buf is not freed in some of the error paths in
brcmf_sdio_read_control(). Fix that by adding vfree() calls.

Fixes: dd43a01c5cdb ("brcmfmac: use dynamically allocated control frame buffer")
Cc: stable@vger.kernel.org
Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
---
Compile tested only. Issue found using static analysis.

 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index b725c64e5b5c..0900211ba2fe 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -1792,6 +1792,7 @@ brcmf_sdio_read_control(struct brcmf_sdio *bus, u8 *hdr, uint len, uint doff)
 		brcmf_err("%d-byte control read exceeds %d-byte buffer\n",
 			  rdlen, bus->sdiodev->bus_if->maxctl);
 		brcmf_sdio_rxfail(bus, false, false);
+		vfree(buf);
 		goto done;
 	}
 
@@ -1800,6 +1801,7 @@ brcmf_sdio_read_control(struct brcmf_sdio *bus, u8 *hdr, uint len, uint doff)
 			  len, len - doff, bus->sdiodev->bus_if->maxctl);
 		bus->sdcnt.rx_toolong++;
 		brcmf_sdio_rxfail(bus, false, false);
+		vfree(buf);
 		goto done;
 	}
 
@@ -1813,6 +1815,7 @@ brcmf_sdio_read_control(struct brcmf_sdio *bus, u8 *hdr, uint len, uint doff)
 			  rdlen, sdret);
 		bus->sdcnt.rxc_errors++;
 		brcmf_sdio_rxfail(bus, true, true);
+		vfree(buf);
 		goto done;
 	} else
 		memcpy(buf + BRCMF_FIRSTREAD, rbuf, rdlen);
-- 
2.43.0

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Arend van Spriel]

On Wed, 22 Jul 2026 16:37:12 +0530, Abdun Nihaal <nihaal@cse.iitm.ac.in> wrote:
> The memory allocated for buf is not freed in some of the error paths in
> brcmf_sdio_read_control(). Fix that by adding vfree() calls.
>
> Fixes: dd43a01c5cdb ("brcmfmac: use dynamically allocated control frame buffer")
> Cc: stable@vger.kernel.org
> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
> ---
> Compile tested only. Issue found using static analysis.
>
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
>  1 file changed, 3 insertions(+)

To be applied to wireless tree.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Regards,
Arend

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Johannes Berg]

On Sun, 2026-08-02 at 10:38 +0200, Arend van Spriel wrote:
> On Wed, 22 Jul 2026 16:37:12 +0530, Abdun Nihaal <nihaal@cse.iitm.ac.in> wrote:
> > The memory allocated for buf is not freed in some of the error paths in
> > brcmf_sdio_read_control(). Fix that by adding vfree() calls.
> > 
> > Fixes: dd43a01c5cdb ("brcmfmac: use dynamically allocated control frame buffer")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
> > ---
> > Compile tested only. Issue found using static analysis.
> > 
> >  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> To be applied to wireless tree.

I'm going to put this into wireless-next, at this point (-rc6) this just
doesn't cut it, the commit message makes it sound purely theoretical.

johannes

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Johannes Berg]

On Sun, 2026-08-02 at 18:41 +0200, Johannes Berg wrote:
> On Sun, 2026-08-02 at 10:38 +0200, Arend van Spriel wrote:
> > On Wed, 22 Jul 2026 16:37:12 +0530, Abdun Nihaal <nihaal@cse.iitm.ac.in> wrote:
> > > The memory allocated for buf is not freed in some of the error paths in
> > > brcmf_sdio_read_control(). Fix that by adding vfree() calls.
> > > 
> > > Fixes: dd43a01c5cdb ("brcmfmac: use dynamically allocated control frame buffer")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
> > > ---
> > > Compile tested only. Issue found using static analysis.
> > > 
> > >  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > 
> > To be applied to wireless tree.
> 
> I'm going to put this into wireless-next, at this point (-rc6) this just
> doesn't cut it, the commit message makes it sound purely theoretical.

Actually, I'm not even going to do that - sprinkling vfree()s over the
code when moving the one that exists would be sufficient is, and I'm
being generous here, not a good change.

johannes

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Arend van Spriel]

Op 2 augustus 2026 18:44:40 schreef Johannes Berg <johannes@sipsolutions.net>:

> On Sun, 2026-08-02 at 18:41 +0200, Johannes Berg wrote:
>> On Sun, 2026-08-02 at 10:38 +0200, Arend van Spriel wrote:
>>> On Wed, 22 Jul 2026 16:37:12 +0530, Abdun Nihaal <nihaal@cse.iitm.ac.in> wrote:
>>>> The memory allocated for buf is not freed in some of the error paths in
>>>> brcmf_sdio_read_control(). Fix that by adding vfree() calls.
>>>>
>>>> Fixes: dd43a01c5cdb ("brcmfmac: use dynamically allocated control frame 
>>>> buffer")
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
>>>> ---
>>>> Compile tested only. Issue found using static analysis.
>>>>
>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>
>>> To be applied to wireless tree.
>>
>> I'm going to put this into wireless-next, at this point (-rc6) this just
>> doesn't cut it, the commit message makes it sound purely theoretical.
>
> Actually, I'm not even going to do that - sprinkling vfree()s over the
> code when moving the one that exists would be sufficient is, and I'm
> being generous here, not a good change.

Sorry, Johannes

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. Maybe better to do:

        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);
}

Gr. AvS

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Arend van Spriel]

On 03/08/2026 07:44, Arend van Spriel wrote:
> Op 2 augustus 2026 18:44:40 schreef Johannes Berg 
> <johannes@sipsolutions.net>:
> 
>> On Sun, 2026-08-02 at 18:41 +0200, Johannes Berg wrote:
>>> On Sun, 2026-08-02 at 10:38 +0200, Arend van Spriel wrote:
>>>> On Wed, 22 Jul 2026 16:37:12 +0530, Abdun Nihaal 
>>>> <nihaal@cse.iitm.ac.in> wrote:
>>>>> The memory allocated for buf is not freed in some of the error 
>>>>> paths in
>>>>> brcmf_sdio_read_control(). Fix that by adding vfree() calls.
>>>>>
>>>>> Fixes: dd43a01c5cdb ("brcmfmac: use dynamically allocated control 
>>>>> frame buffer")
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Abdun Nihaal <nihaal@cse.iitm.ac.in>
>>>>> ---
>>>>> Compile tested only. Issue found using static analysis.
>>>>>
>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 3 +++
>>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> To be applied to wireless tree.
>>>
>>> I'm going to put this into wireless-next, at this point (-rc6) this just
>>> doesn't cut it, the commit message makes it sound purely theoretical.
>>
>> Actually, I'm not even going to do that - sprinkling vfree()s over the
>> code when moving the one that exists would be sufficient is, and I'm
>> being generous here, not a good change.
> 
> Sorry, Johannes
> 
> 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. 
> Maybe better to do:
> 
>         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);
> }
Actually it turns out brcmf_sdio_read_control() is only called once so 
the brcmf_sdio_dcmd_resp_wake() can be taken outside the function.

Gr. AvS

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Johannes Berg]

On Mon, 2026-08-03 at 10:21 +0200, Arend van Spriel wrote:
> > 
> > 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:
> > 
> >         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);
> > }
> Actually it turns out brcmf_sdio_read_control() is only called once so 
> 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?

johannes

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Arend van Spriel]

On 03/08/2026 10:35, Johannes Berg wrote:
> On Mon, 2026-08-03 at 10:21 +0200, Arend van Spriel wrote:
>>>
>>> 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:
>>>
>>>          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);
>>> }
>> Actually it turns out brcmf_sdio_read_control() is only called once so
>> 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?
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?

Gr. AvS

Re: [PATCH] wifi: brcmfmac: Fix memory leak in brcmf_sdio_read_control()

[Johannes Berg]

On Mon, 2026-08-03 at 10:54 +0200, Arend van Spriel wrote:
> > 
> > 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?
> 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 wasn't going to do it myself, but I guess I don't care who does :)

johannes