Received-SPF: pass (google.com: domain of linux-kernel+bounces-76076-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <00000000000043b1310611e388aa@google.com> <20240221131546.GE15988@breakpoint.cc>
In-Reply-To: <20240221131546.GE15988@breakpoint.cc>
From: Eric Dumazet <edumazet@google.com>
Date: Thu, 22 Feb 2024 09:14:15 +0100
Message-ID: <CANn89iK_D+v2J7Ftg1W6-zn7KSZajwWVzfetSdrBPM6f_Zg80A@mail.gmail.com>
Subject: Re: [syzbot] [net?] WARNING in mpls_gso_segment
To: Florian Westphal <fw@strlen.de>
Cc: syzbot <syzbot+99d15fcdb0132a1e1a82@syzkaller.appspotmail.com>, 
	davem@davemloft.net, dsahern@kernel.org, horms@kernel.org, kuba@kernel.org, 
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org, pabeni@redhat.com, 
	syzkaller-bugs@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 21, 2024 at 2:15=E2=80=AFPM Florian Westphal <fw@strlen.de> wro=
te:
>
> syzbot <syzbot+99d15fcdb0132a1e1a82@syzkaller.appspotmail.com> wrote:
> > C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=3D1536462c180=
000
> >
> > Downloadable assets:
> > disk image: https://storage.googleapis.com/syzbot-assets/adbf5d8e38d7/d=
isk-49344462.raw.xz
> > vmlinux: https://storage.googleapis.com/syzbot-assets/0f8e3fb78410/vmli=
nux-49344462.xz
> > kernel image: https://storage.googleapis.com/syzbot-assets/682f4814bf23=
/bzImage-49344462.xz
> >
> > The issue was bisected to:
> >
> > commit 219eee9c0d16f1b754a8b85275854ab17df0850a
> > Author: Florian Westphal <fw@strlen.de>
> > Date:   Fri Feb 16 11:36:57 2024 +0000
> >
> >     net: skbuff: add overflow debug check to pull/push helpers
> >
> > bisection log:  https://syzkaller.appspot.com/x/bisect.txt?x=3D13262752=
180000
> > final oops:     https://syzkaller.appspot.com/x/report.txt?x=3D10a62752=
180000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=3D17262752180=
000
> >
> > IMPORTANT: if you fix the issue, please add the following tag to the co=
mmit:
> > Reported-by: syzbot+99d15fcdb0132a1e1a82@syzkaller.appspotmail.com
> > Fixes: 219eee9c0d16 ("net: skbuff: add overflow debug check to pull/pus=
h helpers")
> >
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 pskb_may_pull_=
reason include/linux/skbuff.h:2723 [inline]
> > WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 pskb_may_pull =
include/linux/skbuff.h:2739 [inline]
> > WARNING: CPU: 0 PID: 5068 at include/linux/skbuff.h:2723 mpls_gso_segme=
nt+0x773/0xaa0 net/mpls/mpls_gso.c:34
>
> Two possible solutions:
>
> 1.)
>
> diff --git a/net/mpls/mpls_gso.c b/net/mpls/mpls_gso.c
> index 533d082f0701..43801b78dd64 100644
> --- a/net/mpls/mpls_gso.c
> +++ b/net/mpls/mpls_gso.c
> @@ -25,12 +25,13 @@ static struct sk_buff *mpls_gso_segment(struct sk_buf=
f *skb,
>         netdev_features_t mpls_features;
>         u16 mac_len =3D skb->mac_len;
>         __be16 mpls_protocol;
> -       unsigned int mpls_hlen;
> +       int mpls_hlen;
>
>         skb_reset_network_header(skb);
>         mpls_hlen =3D skb_inner_network_header(skb) - skb_network_header(=
skb);
> -       if (unlikely(!mpls_hlen || mpls_hlen % MPLS_HLEN))
> +       if (unlikely(mpls_hlen <=3D 0 || mpls_hlen % MPLS_HLEN))
>                 goto out;
> +
>         if (unlikely(!pskb_may_pull(skb, mpls_hlen)))
>                 goto out;

I guess we should try this, or perhaps understand why
skb->encapsulation might not be set,
or why skb_inner_network_header(skb) is not set at this point.

>
> (or a variation thereof).
>
> 2) revert the pskb_may_pull_reason change added in 219eee9c0d16f1b754a8 t=
o
> make it tolerant to "negative" (huge) may-pull requests again.
>
> With above repro, skb_inner_network_header() yields 0, skb_network_header=
()
> returns 108, so we "pskb_may_pull(skb, -108)))" which now triggers
> DEBUG_NET_WARN_ON_ONCE() check.
>
> Before blamed commit, this would make pskb_may_pull hit:
>
>         if (unlikely(len > skb->len))
>                 return SKB_DROP_REASON_PKT_TOO_SMALL;
>
> and mpls_gso_segment takes the 'goto out' label.
>
> So question is really if we should fix this in mpls_gso (and possible oth=
ers
> that try to pull negative numbers...) or if we should legalize this, eith=
er by
> adding explicit if (unlikely(len > INT_MAX)) test to pskb_may_pull_reason=
 or
> by adding a comment that negative 'len' numbers are expected to be caught=
 by
> the check vs. skb->len.
>
> Opinions?

Lets live without 2) for a while, try to fix callers ?