[PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

From: "Peter S. Housel" <housel@acm.org>

An earlier change to this function (3bdae810721b) fixed a leak in the
case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
glom_skb buffer, used for emulating a scattering read, is never used
or referenced after its contents are copied into the destination
buffers, and therefore always needs to be freed by the end of the
function.

Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain")
Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")
Signed-off-by: Peter S. Housel <housel@acm.org>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
 changes:
  V2:
   - avoid goto using if (!err) {}.
  V3:
   - free glom_skb at done label.
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index 9b970dc..844c1e6 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -706,7 +706,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev *sdiodev, struct sk_buff *pkt)
 int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
 			   struct sk_buff_head *pktq, uint totlen)
 {
-	struct sk_buff *glom_skb;
+	struct sk_buff *glom_skb = NULL;
 	struct sk_buff *skb;
 	u32 addr = sdiodev->sbwad;
 	int err = 0;
@@ -727,10 +727,8 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
 			return -ENOMEM;
 		err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
 					 glom_skb);
-		if (err) {
-			brcmu_pkt_buf_free_skb(glom_skb);
+		if (err)
 			goto done;
-		}
 
 		skb_queue_walk(pktq, skb) {
 			memcpy(skb->data, glom_skb->data, skb->len);
@@ -741,6 +739,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
 					    pktq);
 
 done:
+	brcmu_pkt_buf_free_skb(glom_skb);
 	return err;
 }
 
-- 
1.9.1

Re: [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

On 6/12/2017 12:46 PM, Arend van Spriel wrote:
> From: "Peter S. Housel" <housel@acm.org>
> 
> An earlier change to this function (3bdae810721b) fixed a leak in the
> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
> glom_skb buffer, used for emulating a scattering read, is never used
> or referenced after its contents are copied into the destination
> buffers, and therefore always needs to be freed by the end of the
> function.
> 

Kalle,

Can you add stable tag:

Cc: stable@vger.kernel.org # 4.9.x-
> Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain")
> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")
> Signed-off-by: Peter S. Housel <housel@acm.org>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> ---
>   changes:
>    V2:
>     - avoid goto using if (!err) {}.
>    V3:
>     - free glom_skb at done label.
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index 9b970dc..844c1e6 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -706,7 +706,7 @@ int brcmf_sdiod_recv_pkt(struct brcmf_sdio_dev *sdiodev, struct sk_buff *pkt)
>   int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
>   			   struct sk_buff_head *pktq, uint totlen)
>   {
> -	struct sk_buff *glom_skb;
> +	struct sk_buff *glom_skb = NULL;
>   	struct sk_buff *skb;
>   	u32 addr = sdiodev->sbwad;
>   	int err = 0;
> @@ -727,10 +727,8 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
>   			return -ENOMEM;
>   		err = brcmf_sdiod_buffrw(sdiodev, SDIO_FUNC_2, false, addr,
>   					 glom_skb);
> -		if (err) {
> -			brcmu_pkt_buf_free_skb(glom_skb);
> +		if (err)
>   			goto done;
> -		}
>   
>   		skb_queue_walk(pktq, skb) {
>   			memcpy(skb->data, glom_skb->data, skb->len);
> @@ -741,6 +739,7 @@ int brcmf_sdiod_recv_chain(struct brcmf_sdio_dev *sdiodev,
>   					    pktq);
>   
>   done:
> +	brcmu_pkt_buf_free_skb(glom_skb);
>   	return err;
>   }
>   
> 

Re: [PATCH V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 6/12/2017 12:46 PM, Arend van Spriel wrote:
>> From: "Peter S. Housel" <housel@acm.org>
>>
>> An earlier change to this function (3bdae810721b) fixed a leak in the
>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>> glom_skb buffer, used for emulating a scattering read, is never used
>> or referenced after its contents are copied into the destination
>> buffers, and therefore always needs to be freed by the end of the
>> function.
>>
>
> Kalle,
>
> Can you add stable tag:
>
> Cc: stable@vger.kernel.org # 4.9.x-

Yes, will do.

-- 
Kalle Valo

Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Kalle Valo]

Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:

> From: "Peter S. Housel" <housel@acm.org>
> 
> An earlier change to this function (3bdae810721b) fixed a leak in the
> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
> glom_skb buffer, used for emulating a scattering read, is never used
> or referenced after its contents are copied into the destination
> buffers, and therefore always needs to be freed by the end of the
> function.
> 
> Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain")
> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")
> Cc: stable@vger.kernel.org # 4.9.x-
> Signed-off-by: Peter S. Housel <housel@acm.org>
> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>

Patch applied to wireless-drivers-next.git, thanks.

5ea59db8a375 brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

-- 
https://patchwork.kernel.org/patch/9780735/

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

Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

On 13-06-17 09:00, Kalle Valo wrote:
> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
> 
>> From: "Peter S. Housel" <housel@acm.org>
>>
>> An earlier change to this function (3bdae810721b) fixed a leak in the
>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>> glom_skb buffer, used for emulating a scattering read, is never used
>> or referenced after its contents are copied into the destination
>> buffers, and therefore always needs to be freed by the end of the
>> function.
>>
>> Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain")
>> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for host without sg support")
>> Cc: stable@vger.kernel.org # 4.9.x-
>> Signed-off-by: Peter S. Housel <housel@acm.org>
>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> 
> Patch applied to wireless-drivers-next.git, thanks.

Yikes. You say wireless-drivers-next? I should have tagged it better,
but I would like to get this fix in 4.12 and stable.

Regards,
Arend

> 5ea59db8a375 brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain
> 

Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 13-06-17 09:00, Kalle Valo wrote:
>> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
>> 
>>> From: "Peter S. Housel" <housel@acm.org>
>>>
>>> An earlier change to this function (3bdae810721b) fixed a leak in the
>>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>>> glom_skb buffer, used for emulating a scattering read, is never used
>>> or referenced after its contents are copied into the destination
>>> buffers, and therefore always needs to be freed by the end of the
>>> function.
>>>
>>> Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain")
>>> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for
>>> host without sg support")
>>> Cc: stable@vger.kernel.org # 4.9.x-
>>> Signed-off-by: Peter S. Housel <housel@acm.org>
>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> 
>> Patch applied to wireless-drivers-next.git, thanks.
>
> Yikes. You say wireless-drivers-next? I should have tagged it better,
> but I would like to get this fix in 4.12 and stable.

Yes, always document clearly your intentions. I have so many patches
(and emails) to go through that I do not have much time for each patch
to figure out which tree it should go. And in this case the commit log
didn't mention any major breakage so I assumed this is for -next.

In theory I could cherry-pick the commit to wireless-drivers, but as
this doesn't look like a serious issue (no crashes or anything like
that), is it enough that this goes to 4.12 via stable tree? Just takes a
little longer, nothing else.

-- 
Kalle Valo

Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Arend van Spriel]

On 6/14/2017 11:21 AM, Kalle Valo wrote:
> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
> 
>> On 13-06-17 09:00, Kalle Valo wrote:
>>> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
>>>
>>>> From: "Peter S. Housel" <housel@acm.org>
>>>>
>>>> An earlier change to this function (3bdae810721b) fixed a leak in the
>>>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>>>> glom_skb buffer, used for emulating a scattering read, is never used
>>>> or referenced after its contents are copied into the destination
>>>> buffers, and therefore always needs to be freed by the end of the
>>>> function.
>>>>
>>>> Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv_chain")
>>>> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for
>>>> host without sg support")
>>>> Cc: stable@vger.kernel.org # 4.9.x-
>>>> Signed-off-by: Peter S. Housel <housel@acm.org>
>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>
>>> Patch applied to wireless-drivers-next.git, thanks.
>>
>> Yikes. You say wireless-drivers-next? I should have tagged it better,
>> but I would like to get this fix in 4.12 and stable.
> 
> Yes, always document clearly your intentions. I have so many patches
> (and emails) to go through that I do not have much time for each patch
> to figure out which tree it should go. And in this case the commit log
> didn't mention any major breakage so I assumed this is for -next.
> 
> In theory I could cherry-pick the commit to wireless-drivers, but as
> this doesn't look like a serious issue (no crashes or anything like
> that), is it enough that this goes to 4.12 via stable tree? Just takes a
> little longer, nothing else.

It is for you to decide. This is what Peter wrote:

"""
I’m fine with this, or indeed most of the other proposed solutions. The 
important thing is that the leak is fixed; in the driver's current state 
I was able to run our wearable device out of memory in just over 20 
seconds running iperf.
"""

Regards,
Arend

Re: [V3] brcmfmac: Fix glom_skb leak in brcmf_sdiod_recv_chain

[Kalle Valo]

Arend van Spriel <arend.vanspriel@broadcom.com> writes:

> On 6/14/2017 11:21 AM, Kalle Valo wrote:
>> Arend van Spriel <arend.vanspriel@broadcom.com> writes:
>>
>>> On 13-06-17 09:00, Kalle Valo wrote:
>>>> Arend Van Spriel <arend.vanspriel@broadcom.com> wrote:
>>>>
>>>>> From: "Peter S. Housel" <housel@acm.org>
>>>>>
>>>>> An earlier change to this function (3bdae810721b) fixed a leak in the
>>>>> case of an unsuccessful call to brcmf_sdiod_buffrw(). However, the
>>>>> glom_skb buffer, used for emulating a scattering read, is never used
>>>>> or referenced after its contents are copied into the destination
>>>>> buffers, and therefore always needs to be freed by the end of the
>>>>> function.
>>>>>
>>>>> Fixes: 3bdae810721b ("brcmfmac: Fix glob_skb leak in brcmf_sdiod_recv=
_chain")
>>>>> Fixes: a413e39a38573 ("brcmfmac: fix brcmf_sdcard_recv_chain() for
>>>>> host without sg support")
>>>>> Cc: stable@vger.kernel.org # 4.9.x-
>>>>> Signed-off-by: Peter S. Housel <housel@acm.org>
>>>>> Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>>>
>>>> Patch applied to wireless-drivers-next.git, thanks.
>>>
>>> Yikes. You say wireless-drivers-next? I should have tagged it better,
>>> but I would like to get this fix in 4.12 and stable.
>>
>> Yes, always document clearly your intentions. I have so many patches
>> (and emails) to go through that I do not have much time for each patch
>> to figure out which tree it should go. And in this case the commit log
>> didn't mention any major breakage so I assumed this is for -next.
>>
>> In theory I could cherry-pick the commit to wireless-drivers, but as
>> this doesn't look like a serious issue (no crashes or anything like
>> that), is it enough that this goes to 4.12 via stable tree? Just takes a
>> little longer, nothing else.
>
> It is for you to decide. This is what Peter wrote:
>
> """
> I=E2=80=99m fine with this, or indeed most of the other proposed solution=
s.
> The important thing is that the leak is fixed; in the driver's current
> state I was able to run our wearable device out of memory in just over
> 20 seconds running iperf.
> """

Ok, if there's just one report, and even that on a special device,
having the fix go via the stable tree should be fine.

--=20
Kalle Valo