netdev - Re: [PATCH net] bpf: Don't redirect too small packets

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZgGmQu09Z9xN7eOD@google.com>
Date: Mon, 25 Mar 2024 09:28:50 -0700
From: Stanislav Fomichev <sdf@...gle.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Eric Dumazet <edumazet@...gle.com>, Guillaume Nault <gnault@...hat.com>, 
	patchwork-bot+netdevbpf@...nel.org, "David S. Miller" <davem@...emloft.net>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Daniel Borkmann <daniel@...earbox.net>, Alexei Starovoitov <ast@...nel.org>, 
	Martin KaFai Lau <martin.lau@...ux.dev>, Andrii Nakryiko <andrii@...nel.org>, 
	Network Development <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>, 
	Eric Dumazet <eric.dumazet@...il.com>, 
	syzbot+9e27778c0edc62cb97d8@...kaller.appspotmail.com, 
	Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] bpf: Don't redirect too small packets

On 03/25, Alexei Starovoitov wrote:
> On Mon, Mar 25, 2024 at 6:33 AM Eric Dumazet <edumazet@...gle.com> wrote:
> >
> > On Sat, Mar 23, 2024 at 4:02 AM Alexei Starovoitov
> > <alexei.starovoitov@...il.com> wrote:
> > >
> > > On Fri, Mar 22, 2024 at 7:10 AM <patchwork-bot+netdevbpf@...nel.org> wrote:
> > > >
> > > > Hello:
> > > >
> > > > This patch was applied to bpf/bpf.git (master)
> > > > by Daniel Borkmann <daniel@...earbox.net>:
> > > >
> > > > On Fri, 22 Mar 2024 12:24:07 +0000 you wrote:
> > > > > Some drivers ndo_start_xmit() expect a minimal size, as shown
> > > > > by various syzbot reports [1].
> > > > >
> > > > > Willem added in commit 217e6fa24ce2 ("net: introduce device min_header_len")
> > > > > the missing attribute that can be used by upper layers.
> > > > >
> > > > > We need to use it in __bpf_redirect_common().
> > >
> > > This patch broke empty_skb test:
> > > $ test_progs -t empty_skb
> > >
> > > test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
> > > [redirect_ingress] unexpected ret: veth ETH_HLEN+1 packet ingress
> > > [redirect_ingress]: actual -34 != expected 0
> > > test_empty_skb:PASS:err: veth ETH_HLEN+1 packet ingress [redirect_egress] 0 nsec
> > > test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
> > > [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress
> > > [redirect_egress]: actual -34 != expected 1
> > >
> > > And looking at the test I think it's not a test issue.
> > > This check
> > > if (unlikely(skb->len < dev->min_header_len))
> > > is rejecting more than it should.
> > >
> > > So I reverted this patch for now.
> >
> > OK, it seems I missed __bpf_rx_skb() vs __bpf_tx_skb(), but even if I
> > move my sanity test in __bpf_tx_skb(),
> > the bpf test program still fails, I am suspecting the test needs to be adjusted.
> >
> >
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 745697c08acb3a74721d26ee93389efa81e973a0..e9c0e2087a08f1d8afd2c3e8e7871ddc9231b76d
> > 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2128,6 +2128,12 @@ static inline int __bpf_tx_skb(struct
> > net_device *dev, struct sk_buff *skb)
> >                 return -ENETDOWN;
> >         }
> >
> > +       if (unlikely(skb->len < dev->min_header_len)) {
> > +               pr_err_once("__bpf_tx_skb skb->len=%u <
> > dev(%s)->min_header_len(%u)\n", skb->len, dev->name,
> > dev->min_header_len);
> > +               DO_ONCE_LITE(skb_dump, KERN_ERR, skb, false);
> > +               kfree_skb(skb);
> > +               return -ERANGE;
> > +       } // Note: this is before we change skb->dev
> >         skb->dev = dev;
> >         skb_set_redirected_noclear(skb, skb_at_tc_ingress(skb));
> >         skb_clear_tstamp(skb);
> >
> >
> > -->
> >
> >
> > test_empty_skb:FAIL:ret: veth ETH_HLEN+1 packet ingress
> > [redirect_egress] unexpected ret: veth ETH_HLEN+1 packet ingress
> > [redirect_egress]: actual -34 != expected 1
> >
> > [   58.382051] __bpf_tx_skb skb->len=1 < dev(veth0)->min_header_len(14)
> > [   58.382778] skb len=1 headroom=78 headlen=1 tailroom=113
> >                mac=(64,14) net=(78,-1) trans=-1
> >                shinfo(txflags=0 nr_frags=0 gso(size=0 type=0 segs=0))
> >                csum(0x0 ip_summed=0 complete_sw=0 valid=0 level=0)
> >                hash(0x0 sw=0 l4=0) proto=0x7f00 pkttype=0 iif=0
> 
> Hmm. Something is off.
> That test creates 15 byte skb.
> It's not obvious to me how it got reduced to 1.
> Something stripped L2 header and the prog is trying to redirect
> such skb into veth that expects skb with L2 ?
> 
> Stan,
> please take a look.
> Since you wrote that test.

Sure. Daniel wants to take a look on a separate thread, so we can sync
up. Tentatively, seems like the failure is in the lwt path that does
indeed drop the l2.