netdev - Re: [PATCH v3 3/5] vhost_net: remove virtio_net_hdr validation, let tun/tap do it themselves

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]

Date:   Fri, 25 Jun 2021 09:37:34 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Jason Wang <jasowang@...hat.com>, netdev@...r.kernel.org
Cc:     Eugenio Pérez <eperezma@...hat.com>,
        Willem de Bruijn <willemb@...gle.com>
Subject: Re: [PATCH v3 3/5] vhost_net: remove virtio_net_hdr validation, let
 tun/tap do it themselves

On Fri, 2021-06-25 at 15:33 +0800, Jason Wang wrote:
> 在 2021/6/24 下午8:30, David Woodhouse 写道:
> > From: David Woodhouse<dwmw@...zon.co.uk>
> > 
> > When the underlying socket isn't configured with a virtio_net_hdr, the
> > existing code in vhost_net_build_xdp() would attempt to validate
> > uninitialised data, by copying zero bytes (sock_hlen) into the local
> > copy of the header and then trying to validate that.
> > 
> > Fixing it is somewhat non-trivial because the tun device might put a
> > struct tun_pi*before*  the virtio_net_hdr, which makes it hard to find.
> > So just stop messing with someone else's data in vhost_net_build_xdp(),
> > and let tap and tun validate it for themselves, as they do in the
> > non-XDP case anyway.
> 
> 
> Thinking in another way. All XDP stuffs for vhost is prepared for TAP. 
> XDP is not expected to work for TUN.
> 
> So we can simply let's vhost doesn't go with XDP path is the underlayer 
> socket is TUN.

Actually, IFF_TUN mode per se isn't that complex. It's fixed purely on
the tun side by that first patch I posted, which I later expanded a
little to factor out tun_skb_set_protocol().

The next two patches in my original set were fixing up the fact that
XDP currently assumes that the *socket* will be doing the vhdr, not
vhost. Those two weren't tun-specific at all.

It's supporting the PI header (which tun puts *before* the virtio
header as I just said) which introduces a tiny bit more complexity.

So yes, avoiding the XDP path if PI is being used would make some
sense.

In fact I wouldn't be entirely averse to refusing PI mode completely,
as long as we fail gracefully at setup time by refusing the
SET_BACKEND. Not by just silently failing to receive packets.

But then again, it's not actually *that* hard to support, and it's
working fine in my selftests at the end of my patch series.

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (5174 bytes)