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