linux-kernel - Re: [PATCH RFC] uaccess: user_access_begin_after_access

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: <3358ae96-abb6-6be9-346a-0e971cb84dcd@redhat.com>
Date:   Wed, 3 Jun 2020 11:57:11 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     Al Viro <viro@...iv.linux.org.uk>,
        "Michael S. Tsirkin" <mst@...hat.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH RFC] uaccess: user_access_begin_after_access_ok()


On 2020/6/3 上午9:48, Al Viro wrote:
> On Tue, Jun 02, 2020 at 04:45:05AM -0400, Michael S. Tsirkin wrote:
>> So vhost needs to poke at userspace *a lot* in a quick succession.  It
>> is thus benefitial to enable userspace access, do our thing, then
>> disable. Except access_ok has already been pre-validated with all the
>> relevant nospec checks, so we don't need that.  Add an API to allow
>> userspace access after access_ok and barrier_nospec are done.
> BTW, what are you going to do about vq->iotlb != NULL case?  Because
> you sure as hell do *NOT* want e.g. translate_desc() under STAC.
> Disable it around the calls of translate_desc()?


translate_desc() itself doesn't do userspace access, so the idea is 
probably disabling STAC around a batch of vhost_get_uesr()/vhost_put_user().


>
> How widely do you hope to stretch the user_access areas, anyway?


To have best performance for small packets like 64B, if possible, we 
want to disable STAC not only for the metadata access done by vhost 
accessors but also the data access via iov iterator.


>
> BTW, speaking of possible annotations: looks like there's a large
> subset of call graph that can be reached only from vhost_worker()
> or from several ioctls, with all uaccess limited to that subgraph
> (thankfully).  Having that explicitly marked might be a good idea...
>
> Unrelated question, while we are at it: is there any point having
> vhost_get_user() a polymorphic macro?  In all callers the third
> argument is __virtio16 __user * and the second one is an explicit
> *<something> where <something> is __virtio16 *.  Similar for
> vhost_put_user(): in all callers the third arugment is
> __virtio16 __user * and the second - cpu_to_vhost16(vq, something).


This is because all virtqueue metadata that needs to be accessed 
atomically is __virtio16 now, but this may not be true for future virtio 
extension.


>
> Incidentally, who had come up with the name __vhost_get_user?
> Makes for lovey WTF moment for readers - esp. in vhost_put_user()...


I think the confusion comes since it does not accept userspace pointer 
(when IOTLB is enabled).

How about renaming it as vhost_read()/vhost_write() ?

Thanks

>