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

[Ido Schimmel <idosch@nvidia.com>]

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@nvidia.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@syzkaller.appspotmail.com
> > Closes: https://lore.kernel.org/netdev/6735d39a.050a0220.1324f8.0096.GAE@google.com/
> > Signed-off-by: Ido Schimmel <idosch@nvidia.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/