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 <[email protected]>
---
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
Hi Pratyush,
On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <[email protected]> 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 [email protected]?
Thanks,
SJ
> Signed-off-by: Pratyush Yadav <[email protected]>
> ---
>
> 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
>
From: SeongJae Park <[email protected]>
Date: Mon, 22 May 2023 16:55:05 +0000
> Hi Pratyush,
>
> On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <[email protected]> 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 [email protected]?
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 <[email protected]>
> > ---
> >
> > 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
> >
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 <[email protected]> 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 [email protected]?
> >
>
> 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>
On Mon, May 22 2023, SeongJae Park wrote:
> Hi Pratyush,
>
> On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <[email protected]> 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 [email protected]?
>
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/[email protected]/T/#u
>
>
> Thanks,
> SJ
>
>> Signed-off-by: Pratyush Yadav <[email protected]>
>> ---
>>
>> 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
On Mon, 22 May 2023 10:04:30 -0700 Kuniyuki Iwashima <[email protected]> wrote:
> From: SeongJae Park <[email protected]>
> Date: Mon, 22 May 2023 16:55:05 +0000
> > Hi Pratyush,
> >
> > On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <[email protected]> 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 [email protected]?
>
> 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
>
On Mon, 22 May 2023 17:18:53 +0000 SeongJae Park <[email protected]> wrote:
> On Mon, 22 May 2023 10:04:30 -0700 Kuniyuki Iwashima <[email protected]> wrote:
>
> > From: SeongJae Park <[email protected]>
> > Date: Mon, 22 May 2023 16:55:05 +0000
> > > Hi Pratyush,
> > >
> > > On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <[email protected]> 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 [email protected]?
> >
> > 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
> >
On Mon, 22 May 2023 17:23:02 +0000 SeongJae Park <[email protected]> wrote:
> On Mon, 22 May 2023 17:18:53 +0000 SeongJae Park <[email protected]> wrote:
>
> > On Mon, 22 May 2023 10:04:30 -0700 Kuniyuki Iwashima <[email protected]> wrote:
> >
> > > From: SeongJae Park <[email protected]>
> > > Date: Mon, 22 May 2023 16:55:05 +0000
> > > > Hi Pratyush,
> > > >
> > > > On Mon, 22 May 2023 17:30:20 +0200 Pratyush Yadav <[email protected]> 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 [email protected]?
> > >
> > > 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
[...]
On Mon, May 22, 2023 at 7:33 PM SeongJae Park <[email protected]> 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.
Hello:
This patch was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
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