net: fix skb leak in __skb_tstamp_tx()

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

Posted by Pratyush Yadav 2 years, 8 months ago

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(-)

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

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

Posted by patchwork-bot+netdevbpf@kernel.org 2 years, 8 months ago

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

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

Posted by SeongJae Park 2 years, 8 months ago

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
>

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

Posted by Kuniyuki Iwashima 2 years, 8 months ago

From: SeongJae Park <sj@kernel.org>
Date: Mon, 22 May 2023 16:55:05 +0000
> 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?

In netdev, we add 'net' in Subject for bugfix, then netdev maintainers
send a pull request weekly, and stable maintainers backport the fixes to
affected trees.

So we usually need not CC stable for netdev patches.

Thanks,
Kuniyuki

> 
> 
> 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
> >

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

Posted by SeongJae Park 2 years, 8 months ago

On Mon, 22 May 2023 10:04:30 -0700 Kuniyuki Iwashima <kuniyu@amazon.com> wrote:

> From: SeongJae Park <sj@kernel.org>
> Date: Mon, 22 May 2023 16:55:05 +0000
> > 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?
> 
> In netdev, we add 'net' in Subject for bugfix, then netdev maintainers
> send a pull request weekly, and stable maintainers backport the fixes to
> affected trees.
> 
> So we usually need not CC stable for netdev patches.

Thank you for the nice explanation!  Seems it is also well documented at
https://www.kernel.org/doc/html/v5.10/networking/netdev-FAQ.html#q-i-see-a-network-patch-and-i-think-it-should-be-backported-to-stable

However, I don't show the 'net' subject rule on the document.  Is it documented
somewhere else?


Thanks,
SJ

> 
> Thanks,
> Kuniyuki
>

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

Posted by SeongJae Park 2 years, 8 months ago

On Mon, 22 May 2023 17:18:53 +0000 SeongJae Park <sj@kernel.org> wrote:

> On Mon, 22 May 2023 10:04:30 -0700 Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> 
> > From: SeongJae Park <sj@kernel.org>
> > Date: Mon, 22 May 2023 16:55:05 +0000
> > > 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?
> > 
> > In netdev, we add 'net' in Subject for bugfix, then netdev maintainers
> > send a pull request weekly, and stable maintainers backport the fixes to
> > affected trees.
> > 
> > So we usually need not CC stable for netdev patches.
> 
> Thank you for the nice explanation!  Seems it is also well documented at
> https://www.kernel.org/doc/html/v5.10/networking/netdev-FAQ.html#q-i-see-a-network-patch-and-i-think-it-should-be-backported-to-stable
> 
> However, I don't show the 'net' subject rule on the document.  Is it documented
> somewhere else?

Seems I overlooked this:
https://www.kernel.org/doc/html/v5.10/networking/netdev-FAQ.html#q-how-do-i-indicate-which-tree-net-vs-net-next-my-patch-should-be-in


Thanks,
SJ

> 
> 
> Thanks,
> SJ
> 
> > 
> > Thanks,
> > Kuniyuki
> >

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

Posted by SeongJae Park 2 years, 8 months ago

On Mon, 22 May 2023 17:23:02 +0000 SeongJae Park <sj@kernel.org> wrote:

> On Mon, 22 May 2023 17:18:53 +0000 SeongJae Park <sj@kernel.org> wrote:
> 
> > On Mon, 22 May 2023 10:04:30 -0700 Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > 
> > > From: SeongJae Park <sj@kernel.org>
> > > Date: Mon, 22 May 2023 16:55:05 +0000
> > > > 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?
> > > 
> > > In netdev, we add 'net' in Subject for bugfix, then netdev maintainers
> > > send a pull request weekly, and stable maintainers backport the fixes to
> > > affected trees.
> > > 
> > > So we usually need not CC stable for netdev patches.
> > 
> > Thank you for the nice explanation!  Seems it is also well documented at
> > https://www.kernel.org/doc/html/v5.10/networking/netdev-FAQ.html#q-i-see-a-network-patch-and-i-think-it-should-be-backported-to-stable
> > 
> > However, I don't show the 'net' subject rule on the document.  Is it documented
> > somewhere else?
> 
> Seems I overlooked this:
> https://www.kernel.org/doc/html/v5.10/networking/netdev-FAQ.html#q-how-do-i-indicate-which-tree-net-vs-net-next-my-patch-should-be-in

Sorry for continuing adding noises, but seems the process is, or will be,
changed by to the mainline commit dbbe7c962c3a8 ("docs: networking: drop
special stable handling").


Thanks,
SJ

[...]

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

Posted by Eric Dumazet 2 years, 8 months ago

On Mon, May 22, 2023 at 7:33 PM SeongJae Park <sj@kernel.org> wrote:

> Sorry for continuing adding noises, but seems the process is, or will be,
> changed by to the mainline commit dbbe7c962c3a8 ("docs: networking: drop
> special stable handling").

Whoever backported the patch because it had a Fixes: tag will do the
same if another patch also has a Fixes: tag.

I personally prefer Fixes: very precise tags to weak ones.

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

Posted by Pratyush Yadav 2 years, 8 months ago

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
>>

-- 
Regards,
Pratyush Yadav



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879

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

Posted by Greg KH 2 years, 8 months ago

On Mon, May 22, 2023 at 07:03:59PM +0200, Pratyush Yadav wrote:
> 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].


<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>

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

Posted by Kuniyuki Iwashima 2 years, 8 months ago

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

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

Posted by Willem de Bruijn 2 years, 8 months ago

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>