netdev - Re: [PATCH net] vxlan: Avoid accessing uninitialized memory during xmit

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] [day] [month] [year] [list]
Message-ID: <Z2BFZnV2goMfoGh4@shredder>
Date: Mon, 16 Dec 2024 17:21:10 +0200
From: Ido Schimmel <idosch@...dia.com>
To: ericnetdev dumazet <erdnetdev@...il.com>, daniel@...earbox.net,
	sdf@...ichev.me
Cc: Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
	davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
	andrew+netdev@...n.ch, horms@...nel.org, gnault@...hat.com
Subject: Re: [PATCH net] vxlan: Avoid accessing uninitialized memory during
 xmit

On Mon, Dec 16, 2024 at 03:59:12PM +0100, ericnetdev dumazet wrote:
> Le lun. 16 déc. 2024 à 15:44, Ido Schimmel <idosch@...dia.com> a écrit :
> >
> > On Mon, Dec 16, 2024 at 02:48:04PM +0100, Eric Dumazet wrote:
> > > On Mon, Dec 16, 2024 at 2:43 PM Ido Schimmel <idosch@...dia.com> wrote:
> > > >
> > > > The VXLAN driver does not verify that transmitted packets have an
> > > > Ethernet header in the linear part of the skb, which can result in the
> > > > driver accessing uninitialized memory while processing the Ethernet
> > > > header [1]. Issue can be reproduced using [2].
> > > >
> > > > Fix by checking that we can pull the Ethernet header into the linear
> > > > part of the skb. Note that the driver can transmit IP packets, but this
> > > > is handled earlier in the xmit path.
> > > >
> > > > [1]
> > > > CPU: 6 UID: 0 PID: 404 Comm: bpftool Tainted: G    B              6.12.0-rc7-custom-g10d3437464d3 #232
> > > > Tainted: [B]=BAD_PAGE
> > > > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-2.fc40 04/01/2014
> > > > =====================================================
> > > > =====================================================
> > > > BUG: KMSAN: uninit-value in __vxlan_find_mac+0x449/0x450
> > > >  __vxlan_find_mac+0x449/0x450
> > > >  vxlan_xmit+0x1265/0x2f70
> > > >  dev_hard_start_xmit+0x239/0x7e0
> > > >  __dev_queue_xmit+0x2d65/0x45e0
> > > >  __bpf_redirect+0x6d2/0xf60
> > > >  bpf_clone_redirect+0x2c7/0x450
> > > >  bpf_prog_7423975f9f8be99f_mac_repo+0x20/0x22
> > > >  bpf_test_run+0x60f/0xca0
> > > >  bpf_prog_test_run_skb+0x115d/0x2300
> > > >  bpf_prog_test_run+0x3b3/0x5c0
> > > >  __sys_bpf+0x501/0xc60
> > > >  __x64_sys_bpf+0xa8/0xf0
> > > >  do_syscall_64+0xd9/0x1b0
> > > >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > >
> > > > Uninit was stored to memory at:
> > > >  __vxlan_find_mac+0x442/0x450
> > > >  vxlan_xmit+0x1265/0x2f70
> > > >  dev_hard_start_xmit+0x239/0x7e0
> > > >  __dev_queue_xmit+0x2d65/0x45e0
> > > >  __bpf_redirect+0x6d2/0xf60
> > > >  bpf_clone_redirect+0x2c7/0x450
> > > >  bpf_prog_7423975f9f8be99f_mac_repo+0x20/0x22
> > > >  bpf_test_run+0x60f/0xca0
> > > >  bpf_prog_test_run_skb+0x115d/0x2300
> > > >  bpf_prog_test_run+0x3b3/0x5c0
> > > >  __sys_bpf+0x501/0xc60
> > > >  __x64_sys_bpf+0xa8/0xf0
> > > >  do_syscall_64+0xd9/0x1b0
> > > >  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> > > >
> > > > Uninit was created at:
> > > >  kmem_cache_alloc_node_noprof+0x4a8/0x9e0
> > > >  kmalloc_reserve+0xd1/0x420
> > > >  pskb_expand_head+0x1b4/0x15f0
> > > >  skb_ensure_writable+0x2ee/0x390
> > > >  bpf_clone_redirect+0x16a/0x450
> > > >  bpf_prog_7423975f9f8be99f_mac_repo+0x20/0x22
> > > >  bpf_test_run+0x60f/0xca0
> > > >  bpf_prog_test_run_skb+0x115d/0x2300
> > > >  bpf_prog_test_run+0x3b3/0x5c0
> > > >  __sys_bpf+0x501/0xc60
> > > >  __x64_sys_bpf+0xa8/0xf0
> > > >  do_syscall_64+0xd9/0x1b0
> > > >
> > > > [2]
> > > >  $ cat mac_repo.bpf.c
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  #include <linux/bpf.h>
> > > >  #include <bpf/bpf_helpers.h>
> > > >
> > > >  SEC("lwt_xmit")
> > > >  int mac_repo(struct __sk_buff *skb)
> > > >  {
> > > >          return bpf_clone_redirect(skb, 100, 0);
> > > >  }
> > > >
> > > >  $ clang -O2 -target bpf -c mac_repo.bpf.c -o mac_repo.o
> > > >
> > > >  # ip link add name vx0 up index 100 type vxlan id 10010 dstport 4789 local 192.0.2.1
> > > >
> > > >  # bpftool prog load mac_repo.o /sys/fs/bpf/mac_repo
> > > >
> > > >  # echo -ne "\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41\x41" | \
> > > >         bpftool prog run pinned /sys/fs/bpf/mac_repo data_in - repeat 10
> > > >
> > > > Fixes: d342894c5d2f ("vxlan: virtual extensible lan")
> > > > Reported-by: syzbot+35e7e2811bbe5777b20e@...kaller.appspotmail.com
> > > > Closes: https://lore.kernel.org/netdev/6735d39a.050a0220.1324f8.0096.GAE@google.com/
> > > > Signed-off-by: Ido Schimmel <idosch@...dia.com>
> > > > ---
> > > > If this is accepted, I will change dev_core_stats_tx_dropped_inc() to
> > > > dev_dstats_tx_dropped() in net-next.
> > > > ---
> > > >  drivers/net/vxlan/vxlan_core.c | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> > > > index 9ea63059d52d..4cbde7a88205 100644
> > > > --- a/drivers/net/vxlan/vxlan_core.c
> > > > +++ b/drivers/net/vxlan/vxlan_core.c
> > > > @@ -2722,6 +2722,7 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
> > > >         struct vxlan_dev *vxlan = netdev_priv(dev);
> > > >         struct vxlan_rdst *rdst, *fdst = NULL;
> > > >         const struct ip_tunnel_info *info;
> > > > +       enum skb_drop_reason reason;
> > > >         struct vxlan_fdb *f;
> > > >         struct ethhdr *eth;
> > > >         __be32 vni = 0;
> > > > @@ -2746,6 +2747,15 @@ static netdev_tx_t vxlan_xmit(struct sk_buff *skb, struct net_device *dev)
> > > >                 }
> > > >         }
> > > >
> > > > +       reason = pskb_may_pull_reason(skb, ETH_HLEN);
> > > > +       if (unlikely(reason != SKB_NOT_DROPPED_YET)) {
> > > > +               dev_core_stats_tx_dropped_inc(dev);
> > > > +               vxlan_vnifilter_count(vxlan, vni, NULL,
> > > > +                                     VXLAN_VNI_STATS_TX_DROPS, 0);
> > > > +               kfree_skb_reason(skb, reason);
> > > > +               return NETDEV_TX_OK;
> > > > +       }
> > >
> > > I think the plan was to use dev->min_header_len, in the generic part
> > > of networking stack,
> > > instead of having to copy/paste this code in all drivers.
> >
> > Are you referring to [1]? Tested it using the reproducer I mentioned and
> > it seems to work. Is it still blocked by the empty_skb test?
> >
> > [1] https://lore.kernel.org/netdev/20240322122407.1329861-1-edumazet@google.com/
> 
> Yes, I am referring to a generic test done in locations where a
> malicious skb could be cooked/transformed.

OK, thanks Eric. I agree it's a better approach. Looks like it will
allow us to revert 8bd67ebb50c0 ("net: bridge: xmit: make sure we have
at least eth header len bytes") and other patches I might have missed.

Daniel / Stan, are you guys still planning to adjust the empty_skb test
so that Eric's patch could be applied without failing the CI?