netdev - Re: [PATCH] net: tun: fix tun_xdp_one() for IFF

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: <ecc93a78-ee8b-bcf2-9b45-dd98d2f44768@redhat.com>
Date:   Wed, 23 Jun 2021 11:39:50 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     David Woodhouse <dwmw2@...radead.org>,
        netdev <netdev@...r.kernel.org>
Cc:     Eugenio Pérez <eperezma@...hat.com>
Subject: Re: [PATCH] net: tun: fix tun_xdp_one() for IFF_TUN mode


在 2021/6/22 下午4:29, David Woodhouse 写道:
> On Tue, 2021-06-22 at 16:00 +0800, Jason Wang wrote:
>>> I'm tempted to add a new feature for that 1:1 access, with no ->umem or
>>> ->iotlb at all. And then I can use it as a key to know that the XDP
>>> bugs are fixed too :)
>>
>> This means we need validate the userspace address each time before vhost
>> tries to use that. This will de-gradate the performance. So we still
>> need to figure out the legal userspace address range which might not be
>> easy.
> Easier from the kernel than from userspace though :)


Yes.


>
> But I don't know that we need it. Isn't a call to access_ok() going to
> be faster than what translate_desc() does to look things up anyway?


Right.


>
> In the 1:1 mode, the access_ok() is all that's needed since there's no
> translation.
>
> @@ -2038,6 +2065,14 @@ static int translate_desc(struct vhost_virtqueue *vq, u64 addr, u32 len,
>          u64 s = 0;
>          int ret = 0;
>   
> +       if (vhost_has_feature(vq, VHOST_F_IDENTITY_MAPPING)) {


Using vhost_has_feature() is kind of tricky since it's used for virtio 
feature negotiation.

We probably need to use backend_features instead.

I think we should probably do more:

1) forbid the feature to be set when mem table / IOTLB has at least one 
mapping
2) forbid the mem table / IOTLB updating after the feature is set

Thanks


> +               if (!access_ok((void __user *)addr, len))
> +                       return -EFAULT;
> +
> +               iov[0].iov_len = len;
> +               iov[0].iov_base = (void __user *)addr;
> +               return 1;
> +       }
>          while ((u64)len > s) {
>                  u64 size;
>                  if (unlikely(ret >= iov_size)) {