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: <CANn89iLSOeFGNogYMHbeLRC5kOwwArMz3d5_2hZmBn6fibyUhw@mail.gmail.com>
Date: Mon, 25 Mar 2024 14:33:01 +0100
From: Eric Dumazet <edumazet@...gle.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>, Guillaume Nault <gnault@...hat.com>
Cc: 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, 
	Stanislav Fomichev <sdf@...gle.com>, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH net] bpf: Don't redirect too small packets

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

Note that veth driver is one of the few 'Ethernet' drivers that make
sure to get at least 14 bytes in the skb at ndo_start_xmit()
after commit 726e2c5929de841fdcef4e2bf995680688ae1b87 ("veth: Ensure
eth header is in skb's linear part")

BTW this last patch (changing veth) should have been done generically
from act_mirred

(We do not want to patch ~400 ethernet drivers in the tree)