Re: [BUG] vhost/vringh: __get_user without access_ok leaks kernel memory via vDPA passthrough

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Oct 20, 2026 at 11:22:59AM +0200, Eugenio Perez Martin wrote:
> On Tue, Oct 20, 2026 at 8:31 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Oct 19, 2026 at 03:14:46PM +0800, Wang Haoran wrote:
> > > Eugenio previously offered some additional suggestions in reply to
> > > my patch email. Should these also be addressed?
> >
> > The following seems to work for me. Testing would be appreciated.
> >
> > From: Michael S. Tsirkin <mst@redhat.com>
> > Subject: [PATCH v2] vringh: use get_user/put_user in the use_va path
> >
> > getu16_iotlb()/putu16_iotlb() pass the address produced by
> > iotlb_translate() straight to __get_user()/__put_user(),
> > without access_ok checked anywhere. Not good.
> >
> > Switch to get_user()/put_user(), which are always
> > correct, and safer.
> >
> > But the fact is that accessing user memory like this
> > will sometimes (rarely) race with page being swapped
> > out, trigger EFAULT, and it should be retried
> > from a worker, and vdpa_sim currently does not do this.
> >
> > Fixes: 42823a871fd4 ("vringh: support VA with iotlb")
> > Cc: Stefano Garzarella <sgarzare@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ---
> >
> > Notes (changelog):
> > v1->v2:
> >     Use get_user()/put_user() wrapped in pagefault_disable() instead of
> >     access_ok() followed by __get_user()/__put_user(); the helpers are
> >     reached from BH-disabled sections, and __get_user() is not a valid
> >     way around that (David Laight, Linus Torvalds)
> >
> >  drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 2 ++
> >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 2 ++
> >  drivers/vhost/vringh.c               | 4 ++--
> >  3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > index 76dd5b0828d7..325ec8f0ab85 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
> > @@ -347,8 +347,10 @@ static void vdpasim_blk_work(struct vdpasim *vdpasim)
> >                         smp_wmb();
> >
> >                         local_bh_disable();
> > +                       pagefault_disable();
> >                         if (vringh_need_notify_iotlb(&vq->vring) > 0)
> >                                 vringh_notify(&vq->vring);
> > +                       pagefault_enable();
> >                         local_bh_enable();
> >
> >                         if (++reqs > 4) {
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index a6514b5ccd86..964cdf422467 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -77,8 +77,10 @@ static void vdpasim_net_complete(struct vdpasim_virtqueue *vq, size_t len)
> >         smp_wmb();
> >
> >         local_bh_disable();
> > +       pagefault_disable();
> >         if (vringh_need_notify_iotlb(&vq->vring) > 0)
> >                 vringh_notify(&vq->vring);
> > +       pagefault_enable();
> >         local_bh_enable();
> >  }
> >
> 
> Why not move the pagefault disable_enable to wrap just the accesses?

We want most accessed with PF enabled, so memory can be faulted in.

> Future vdpa drivers could use va and we might miss the call. Also, the
> reverse is also true, vdpa_sim could be configured to not use it, even
> if it is not a big waste.
> 
> > diff --git a/drivers/vhost/vringh.c b/drivers/vhost/vringh.c
> > index 9066f9f12ba1..74836eb60c7a 100644
> > --- a/drivers/vhost/vringh.c
> > +++ b/drivers/vhost/vringh.c
> > @@ -1213,7 +1213,7 @@ static inline int getu16_iotlb(const struct vringh *vrh,
> >                 return ret;
> >
> >         if (vrh->use_va) {
> > -               ret = __get_user(tmp, (__virtio16 __user *)ivec.iov.iovec[0].iov_base);
> > +               ret = get_user(tmp, (__virtio16 __user *)ivec.iov.iovec[0].iov_base);
> >                 if (ret)
> >                         return ret;
> >         } else {
> > @@ -1251,7 +1251,7 @@ static inline int putu16_iotlb(const struct vringh *vrh,
> >         tmp = cpu_to_vringh16(vrh, val);
> >
> >         if (vrh->use_va) {
> > -               ret = __put_user(tmp, (__virtio16 __user *)ivec.iov.iovec[0].iov_base);
> > +               ret = put_user(tmp, (__virtio16 __user *)ivec.iov.iovec[0].iov_base);
> >                 if (ret)
> >                         return ret;
> >         } else {
> >