diff mbox series

[net] net: fix skb leak in __skb_tstamp_tx()

Message ID	20230522153020.32422-1-ptyadav@amazon.de
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Pratyush Yadav <ptyadav@amazon.de> To: "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com> CC: Pratyush Yadav <ptyadav@amazon.de>, Kuniyuki Iwashima <kuniyu@amazon.com>, Willem de Bruijn <willemb@google.com>, Norbert Manthey <nmanthey@amazon.de>, <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org> Subject: [PATCH net] net: fix skb leak in __skb_tstamp_tx() Date: Mon, 22 May 2023 17:30:20 +0200 Message-ID: <20230522153020.32422-1-ptyadav@amazon.de> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain Precedence: bulk
Series	[net] net: fix skb leak in __skb_tstamp_tx() \| [net] net: fix skb leak in __skb_tstamp_tx()

Commit Message

Pratyush Yadav May 22, 2023, 3:30 p.m. UTC

  Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
zerocopy skbs. But it ended up adding a leak of its own. When
skb_orphan_frags_rx() fails, the function just returns, leaking the skb
it just cloned. Free it before returning.

This bug was discovered and resolved using Coverity Static Analysis
Security Testing (SAST) by Synopsys, Inc.

Fixes: 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.")
Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>
---

I do not know this code very well, this was caught by our static
analysis tool. I did not try specifically reproducing the leak but I did
do a boot test by adding this patch on 6.4-rc3 and the kernel boots
fine.

 net/core/skbuff.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

--
2.39.2

Comments

Kuniyuki Iwashima May 22, 2023, 3:45 p.m. UTC | #1

From: Pratyush Yadav <ptyadav@amazon.de>
Date: Mon, 22 May 2023 17:30:20 +0200
> Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
> TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
> zerocopy skbs. But it ended up adding a leak of its own. When
> skb_orphan_frags_rx() fails, the function just returns, leaking the skb
> it just cloned. Free it before returning.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.")
> Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Good catch, thanks!


> ---
> 
> I do not know this code very well, this was caught by our static
> analysis tool. I did not try specifically reproducing the leak but I did
> do a boot test by adding this patch on 6.4-rc3 and the kernel boots
> fine.
> 
>  net/core/skbuff.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 515ec5cdc79c..cea28d30abb5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5224,8 +5224,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>  	} else {
>  		skb = skb_clone(orig_skb, GFP_ATOMIC);
> 
> -		if (skb_orphan_frags_rx(skb, GFP_ATOMIC))
> +		if (skb_orphan_frags_rx(skb, GFP_ATOMIC)) {
> +			kfree_skb(skb);
>  			return;
> +		}
>  	}
>  	if (!skb)
>  		return;
> --
> 2.39.2

Willem de Bruijn May 22, 2023, 4:11 p.m. UTC | #2

On Mon, May 22, 2023 at 11:46 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Pratyush Yadav <ptyadav@amazon.de>
> Date: Mon, 22 May 2023 17:30:20 +0200
> > Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
> > TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
> > zerocopy skbs. But it ended up adding a leak of its own. When
> > skb_orphan_frags_rx() fails, the function just returns, leaking the skb
> > it just cloned. Free it before returning.
> >
> > This bug was discovered and resolved using Coverity Static Analysis
> > Security Testing (SAST) by Synopsys, Inc.
> >
> > Fixes: 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.")
> > Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

SeongJae Park May 22, 2023, 4:55 p.m. UTC | #3

Hi Pratyush,

On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <ptyadav@amazon.de> wrote:

> Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
> TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
> zerocopy skbs. But it ended up adding a leak of its own. When
> skb_orphan_frags_rx() fails, the function just returns, leaking the skb
> it just cloned. Free it before returning.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> Fixes: 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.")

Seems the commit has merged in several stable kernels.  Is the bug also
affecting those?  If so, would it be better to Cc stable@vger.kernel.org?


Thanks,
SJ

> Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>
> ---
> 
> I do not know this code very well, this was caught by our static
> analysis tool. I did not try specifically reproducing the leak but I did
> do a boot test by adding this patch on 6.4-rc3 and the kernel boots
> fine.
> 
>  net/core/skbuff.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 515ec5cdc79c..cea28d30abb5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -5224,8 +5224,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>  	} else {
>  		skb = skb_clone(orig_skb, GFP_ATOMIC);
> 
> -		if (skb_orphan_frags_rx(skb, GFP_ATOMIC))
> +		if (skb_orphan_frags_rx(skb, GFP_ATOMIC)) {
> +			kfree_skb(skb);
>  			return;
> +		}
>  	}
>  	if (!skb)
>  		return;
> --
> 2.39.2
>

Pratyush Yadav May 22, 2023, 5:03 p.m. UTC | #4

On Mon, May 22 2023, SeongJae Park wrote:

> Hi Pratyush,
>
> On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <ptyadav@amazon.de> wrote:
>
>> Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
>> TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
>> zerocopy skbs. But it ended up adding a leak of its own. When
>> skb_orphan_frags_rx() fails, the function just returns, leaking the skb
>> it just cloned. Free it before returning.
>>
>> This bug was discovered and resolved using Coverity Static Analysis
>> Security Testing (SAST) by Synopsys, Inc.
>>
>> Fixes: 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with TX timestamp.")
>
> Seems the commit has merged in several stable kernels.  Is the bug also
> affecting those?  If so, would it be better to Cc stable@vger.kernel.org?
>

It affects v5.4.243 at least, since that is where I first saw this. But
I would expect it to affect other stable kernels it has been backported
to as well. I thought using the Fixes tag pointing to the bad upstream
commit would be enough for the stable maintainers' tooling/bots to pick
this patch up.

In either case, +Cc stable. Link to the patch this thread is talking
about [0].

[0] https://lore.kernel.org/netdev/20230522153020.32422-1-ptyadav@amazon.de/T/#u

>
>
> Thanks,
> SJ
>
>> Signed-off-by: Pratyush Yadav <ptyadav@amazon.de>
>> ---
>>
>> I do not know this code very well, this was caught by our static
>> analysis tool. I did not try specifically reproducing the leak but I did
>> do a boot test by adding this patch on 6.4-rc3 and the kernel boots
>> fine.
>>
>>  net/core/skbuff.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
>> index 515ec5cdc79c..cea28d30abb5 100644
>> --- a/net/core/skbuff.c
>> +++ b/net/core/skbuff.c
>> @@ -5224,8 +5224,10 @@ void __skb_tstamp_tx(struct sk_buff *orig_skb,
>>       } else {
>>               skb = skb_clone(orig_skb, GFP_ATOMIC);
>>
>> -             if (skb_orphan_frags_rx(skb, GFP_ATOMIC))
>> +             if (skb_orphan_frags_rx(skb, GFP_ATOMIC)) {
>> +                     kfree_skb(skb);
>>                       return;
>> +             }
>>       }
>>       if (!skb)
>>               return;
>> --
>> 2.39.2
>>

patchwork-bot+netdevbpf@kernel.org May 24, 2023, 4:20 a.m. UTC | #5

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 22 May 2023 17:30:20 +0200 you wrote:
> Commit 50749f2dd685 ("tcp/udp: Fix memleaks of sk and zerocopy skbs with
> TX timestamp.") added a call to skb_orphan_frags_rx() to fix leaks with
> zerocopy skbs. But it ended up adding a leak of its own. When
> skb_orphan_frags_rx() fails, the function just returns, leaking the skb
> it just cloned. Free it before returning.
> 
> This bug was discovered and resolved using Coverity Static Analysis
> Security Testing (SAST) by Synopsys, Inc.
> 
> [...]

Here is the summary with links:
  - [net] net: fix skb leak in __skb_tstamp_tx()
    https://git.kernel.org/netdev/net/c/8a02fb71d719

You are awesome, thank you!

diff mbox series

Patch

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 515ec5cdc79c..cea28d30abb5 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5224,8 +5224,10 @@  void __skb_tstamp_tx(struct sk_buff *orig_skb,
 	} else {
 		skb = skb_clone(orig_skb, GFP_ATOMIC);

-		if (skb_orphan_frags_rx(skb, GFP_ATOMIC))
+		if (skb_orphan_frags_rx(skb, GFP_ATOMIC)) {
+			kfree_skb(skb);
 			return;
+		}
 	}
 	if (!skb)
 		return;