[Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Cong Wang]

From: Cong Wang <cong.wang@bytedance.com>

Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
helpers") introduced an overflow debug check for pull/push helpers.
For __skb_pull() this makes sense because its callers rarely check its
return value. But for pskb_may_pull() it does not make sense, since its
return value is properly taken care of. Remove the one in
pskb_may_pull(), we can continue rely on its return value.

Fixes: 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")
Reported-by: syzbot+0c4150bff9fff3bf023c@syzkaller.appspotmail.com
Cc: Florian Westphal <fw@strlen.de>
Signed-off-by: Cong Wang <cong.wang@bytedance.com>
---
 include/linux/skbuff.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1c2902eaebd3..9fd49bab6595 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2735,8 +2735,6 @@ void *__pskb_pull_tail(struct sk_buff *skb, int delta);
 static inline enum skb_drop_reason
 pskb_may_pull_reason(struct sk_buff *skb, unsigned int len)
 {
-	DEBUG_NET_WARN_ON_ONCE(len > INT_MAX);
-
 	if (likely(len <= skb_headlen(skb)))
 		return SKB_NOT_DROPPED_YET;
 
-- 
2.34.1

Re: [Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Florian Westphal]

Cong Wang <xiyou.wangcong@gmail.com> wrote:
> From: Cong Wang <cong.wang@bytedance.com>
> 
> Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
> helpers") introduced an overflow debug check for pull/push helpers.
> For __skb_pull() this makes sense because its callers rarely check its
> return value. But for pskb_may_pull() it does not make sense, since its
> return value is properly taken care of. Remove the one in
> pskb_may_pull(), we can continue rely on its return value.

See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist
without this check, I would not give up yet.

bpf_try_make_writable() could do an explicit check vs. skb->len.

If anyone needs it, splat is at
https://syzkaller.appspot.com/bug?extid=0c4150bff9fff3bf023c

Re: [Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Cong Wang]

On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > From: Cong Wang <cong.wang@bytedance.com>
> > 
> > Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
> > helpers") introduced an overflow debug check for pull/push helpers.
> > For __skb_pull() this makes sense because its callers rarely check its
> > return value. But for pskb_may_pull() it does not make sense, since its
> > return value is properly taken care of. Remove the one in
> > pskb_may_pull(), we can continue rely on its return value.
> 
> See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist
> without this check, I would not give up yet.

What's the point of that commit?

The "fix" (I doubt it fixes anything) you had is merely exiting a few
lines earlier than pskb_may_pull():

 30         if (!skb_inner_network_header_was_set(skb))
 31                 goto out;
 32 
 33         skb_reset_network_header(skb);
 34         mpls_hlen = skb_inner_network_header(skb) - skb_network_header(skb);
 35         if (unlikely(!mpls_hlen || mpls_hlen % MPLS_HLEN))
 36                 goto out;
 37         if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
 38                 goto out;

Before your "fix", we exit on line 37. After your "fix", we exit on line
30. I don't see any difference here, it returns -EINVAL anyway.

> 
> bpf_try_make_writable() could do an explicit check vs. skb->len.

But why? I don't see the point of its existence. pskb_may_pull() already
checks it very well:

2741         if (unlikely(len > skb->len))
2742                 return SKB_DROP_REASON_PKT_TOO_SMALL;

Thanks.

Re: [Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Kuniyuki Iwashima]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Fri, 7 Jun 2024 09:14:04 -0700
> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > From: Cong Wang <cong.wang@bytedance.com>
> > > 
> > > Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
> > > helpers") introduced an overflow debug check for pull/push helpers.
> > > For __skb_pull() this makes sense because its callers rarely check its
> > > return value. But for pskb_may_pull() it does not make sense, since its
> > > return value is properly taken care of. Remove the one in
> > > pskb_may_pull(), we can continue rely on its return value.
> > 
> > See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist
> > without this check, I would not give up yet.
> 
> What's the point of that commit?

4b911a9690d7 would be better example.  The warning actually found a
bug in NSH GSO.

Here's splats triggered by syzkaller using NSH over various tunnels.
https://lore.kernel.org/netdev/20240415222041.18537-2-kuniyu@amazon.com/

Re: [Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Eric Dumazet]

On 6/7/24 23:32, Kuniyuki Iwashima wrote:
> From: Cong Wang <xiyou.wangcong@gmail.com>
> Date: Fri, 7 Jun 2024 09:14:04 -0700
>> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
>>> Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> From: Cong Wang <cong.wang@bytedance.com>
>>>>
>>>> Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
>>>> helpers") introduced an overflow debug check for pull/push helpers.
>>>> For __skb_pull() this makes sense because its callers rarely check its
>>>> return value. But for pskb_may_pull() it does not make sense, since its
>>>> return value is properly taken care of. Remove the one in
>>>> pskb_may_pull(), we can continue rely on its return value.
>>> See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist
>>> without this check, I would not give up yet.
>> What's the point of that commit?
> 4b911a9690d7 would be better example.  The warning actually found a
> bug in NSH GSO.
>
> Here's splats triggered by syzkaller using NSH over various tunnels.
> https://lore.kernel.org/netdev/20240415222041.18537-2-kuniyu@amazon.com/


Right. We discussed this before. I guess I forgot to send the fix.

Florian could you submit the suggestion I made before ?

diff --git a/net/core/filter.c b/net/core/filter.c
index 
358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2 
100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
  static inline int __bpf_try_make_writable(struct sk_buff *skb,
                        unsigned int write_len)
  {
+#if defined(CONFIG_DEBUG_NET)
+    /* Avoid a splat in pskb_may_pull_reason() */
+    if (write_len > INT_MAX)
+        return -EINVAL;
+#endif
      return skb_ensure_writable(skb, write_len);
  }

Re: [Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Florian Westphal]

Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On 6/7/24 23:32, Kuniyuki Iwashima wrote:
> > From: Cong Wang <xiyou.wangcong@gmail.com>
> > Date: Fri, 7 Jun 2024 09:14:04 -0700
> > > On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> > > > Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > From: Cong Wang <cong.wang@bytedance.com>
> > > > > 
> > > > > Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
> > > > > helpers") introduced an overflow debug check for pull/push helpers.
> > > > > For __skb_pull() this makes sense because its callers rarely check its
> > > > > return value. But for pskb_may_pull() it does not make sense, since its
> > > > > return value is properly taken care of. Remove the one in
> > > > > pskb_may_pull(), we can continue rely on its return value.
> > > > See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist
> > > > without this check, I would not give up yet.
> > > What's the point of that commit?
> > 4b911a9690d7 would be better example.  The warning actually found a
> > bug in NSH GSO.
> > 
> > Here's splats triggered by syzkaller using NSH over various tunnels.
> > https://lore.kernel.org/netdev/20240415222041.18537-2-kuniyu@amazon.com/
> 
> 
> Right. We discussed this before. I guess I forgot to send the fix.
> Florian could you submit the suggestion I made before ?
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2
> 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
>  static inline int __bpf_try_make_writable(struct sk_buff *skb,
>                        unsigned int write_len)
>  {
> +#if defined(CONFIG_DEBUG_NET)
> +    /* Avoid a splat in pskb_may_pull_reason() */
> +    if (write_len > INT_MAX)
> +        return -EINVAL;
> +#endif
>      return skb_ensure_writable(skb, write_len);
>  }

Makes sense, I'll probably not get to this before Friday though, so if
anyone else wants to do this: go right ahead.

Re: [Patch net] net: remove the bogus overflow debug check in pskb_may_pull()

[Jason Xing]

Hello Eric,

On Sat, Jun 8, 2024 at 4:01 PM Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> On 6/7/24 23:32, Kuniyuki Iwashima wrote:
> > From: Cong Wang <xiyou.wangcong@gmail.com>
> > Date: Fri, 7 Jun 2024 09:14:04 -0700
> >> On Fri, Jun 07, 2024 at 01:27:47AM +0200, Florian Westphal wrote:
> >>> Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >>>> From: Cong Wang <cong.wang@bytedance.com>
> >>>>
> >>>> Commit 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push
> >>>> helpers") introduced an overflow debug check for pull/push helpers.
> >>>> For __skb_pull() this makes sense because its callers rarely check its
> >>>> return value. But for pskb_may_pull() it does not make sense, since its
> >>>> return value is properly taken care of. Remove the one in
> >>>> pskb_may_pull(), we can continue rely on its return value.
> >>> See 025f8ad20f2e3264d11683aa9cbbf0083eefbdcd which would not exist
> >>> without this check, I would not give up yet.
> >> What's the point of that commit?
> > 4b911a9690d7 would be better example.  The warning actually found a
> > bug in NSH GSO.
> >
> > Here's splats triggered by syzkaller using NSH over various tunnels.
> > https://lore.kernel.org/netdev/20240415222041.18537-2-kuniyu@amazon.com/
>
>
> Right. We discussed this before. I guess I forgot to send the fix.
>
> Florian could you submit the suggestion I made before ?
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index
> 358870408a51e61f3cbc552736806e4dfee1ec39..da7aae6fd8ba557c66699d1cfebd47f18f442aa2
> 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -1662,6 +1662,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
>   static inline int __bpf_try_make_writable(struct sk_buff *skb,
>                         unsigned int write_len)
>   {
> +#if defined(CONFIG_DEBUG_NET)

I wonder why we want to avoid printing warning information especially
when CONFIG_DEBUG_NET is on? Any reasons?

> +    /* Avoid a splat in pskb_may_pull_reason() */
> +    if (write_len > INT_MAX)

I guess: if the purpose is to skip pskb_may_pull_reason and then
return a proper value (-EINVAL) to its caller when the above statement
is true, can we add the warning here on top of you patch:
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX);
?

After this, we can print useful information and let the caller catch
the -EINVAL return value.

Thanks,
Jason

> +        return -EINVAL;
> +#endif
>       return skb_ensure_writable(skb, write_len);
>   }
>
>
>

[PATCH bpf] bpf: avoid splat in pskb_pull_reason

[Florian Westphal]

syzkaller builds (CONFIG_DEBUG_NET=y) frequently trigger a debug
hint in pskb_may_pull.

We'd like to retain this debug check because it might hint at integer
overflows and other issues (kernel code should pull headers, not huge
value).

In bpf case, this splat isn't interesting at all: such (nonsensical) bpf
programs are typically generated by a fuzzer anyway.

Do what Eric suggested and suppress such warning.

For CONFIG_DEBUG_NET=n we don't need the extra check because
pskb_may_pull will do the right thing: return an error without the
WARN() backtrace.

Reported-by: syzbot+0c4150bff9fff3bf023c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=0c4150bff9fff3bf023c
Fixes: 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")
Link: https://lore.kernel.org/netdev/9f254c96-54f2-4457-b7ab-1d9f6187939c@gmail.com/
Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Florian Westphal <fw@strlen.de>
---
 net/core/filter.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/filter.c b/net/core/filter.c
index 2510464692af..9933851c685e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1665,6 +1665,11 @@ static DEFINE_PER_CPU(struct bpf_scratchpad, bpf_sp);
 static inline int __bpf_try_make_writable(struct sk_buff *skb,
 					  unsigned int write_len)
 {
+#ifdef CONFIG_DEBUG_NET
+	/* Avoid a splat in pskb_may_pull_reason() */
+	if (write_len > INT_MAX)
+		return -EINVAL;
+#endif
 	return skb_ensure_writable(skb, write_len);
 }
 
-- 
2.44.2

Re: [PATCH bpf] bpf: avoid splat in pskb_pull_reason

[Eric Dumazet]

On Fri, Jun 14, 2024 at 12:41 PM Florian Westphal <fw@strlen.de> wrote:
>
> syzkaller builds (CONFIG_DEBUG_NET=y) frequently trigger a debug
> hint in pskb_may_pull.
>
> We'd like to retain this debug check because it might hint at integer
> overflows and other issues (kernel code should pull headers, not huge
> value).
>
> In bpf case, this splat isn't interesting at all: such (nonsensical) bpf
> programs are typically generated by a fuzzer anyway.
>
> Do what Eric suggested and suppress such warning.
>
> For CONFIG_DEBUG_NET=n we don't need the extra check because
> pskb_may_pull will do the right thing: return an error without the
> WARN() backtrace.
>
> Reported-by: syzbot+0c4150bff9fff3bf023c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=0c4150bff9fff3bf023c
> Fixes: 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/push helpers")
> Link: https://lore.kernel.org/netdev/9f254c96-54f2-4457-b7ab-1d9f6187939c@gmail.com/
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---

Thanks Florian

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH bpf] bpf: avoid splat in pskb_pull_reason

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to bpf/bpf.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:

On Fri, 14 Jun 2024 12:17:33 +0200 you wrote:
> syzkaller builds (CONFIG_DEBUG_NET=y) frequently trigger a debug
> hint in pskb_may_pull.
> 
> We'd like to retain this debug check because it might hint at integer
> overflows and other issues (kernel code should pull headers, not huge
> value).
> 
> [...]

Here is the summary with links:
  - [bpf] bpf: avoid splat in pskb_pull_reason
    https://git.kernel.org/bpf/bpf/c/2bbe3e5a2f4e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html