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: <CAADnVQ+OhsBetPT0avuNVsEwru13UtMjX1U_6_u6xROXBBn-Yg@mail.gmail.com>
Date: Mon, 25 Mar 2024 08:56:38 -0700
From: Alexei Starovoitov <alexei.starovoitov@...il.com>
To: Eric Dumazet <edumazet@...gle.com>, Stanislav Fomichev <sdf@...gle.com>
Cc: 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 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.