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:   Mon, 28 Jun 2021 12:23:13 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     David Woodhouse <dwmw2@...radead.org>, 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


在 2021/6/25 下午4:37, David Woodhouse 写道:
> 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.


This reminds me we need to fix tun_put_user_xdp(), but as we've 
discussed, we need first figure out if PI is worth to support for vhost-net.


>
> 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.


That's another way. Actually, macvtap mandate IFF_TAP | IFF_NO_PI.

Thanks


>
> 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.
>