Re: [PATCH RFC 3/4] vhost-vdpa: should restore 1:1 dma mapping before detaching driver

[Si-Wei Liu]

On 8/14/2023 7:32 PM, Jason Wang wrote:
> On Tue, Aug 15, 2023 at 9:45 AM Si-Wei Liu <si-wei.liu@xxxxxxxxxx> wrote:
> > Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> > ---
> >   drivers/vhost/vdpa.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> > index b43e868..62b0a01 100644
> > --- a/drivers/vhost/vdpa.c
> > +++ b/drivers/vhost/vdpa.c
> > @@ -131,6 +131,15 @@ static struct vhost_vdpa_as *vhost_vdpa_find_alloc_as(struct vhost_vdpa *v,
> >          return vhost_vdpa_alloc_as(v, asid);
> >   }
> >
> > +static void vhost_vdpa_reset_map(struct vhost_vdpa *v, u32 asid)
> > +{
> > +       struct vdpa_device *vdpa = v->vdpa;
> > +       const struct vdpa_config_ops *ops = vdpa->config;
> > +
> > +       if (ops->reset_map)
> > +               ops->reset_map(vdpa, asid);
> > +}
> > +
> >   static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> >   {
> >          struct vhost_vdpa_as *as = asid_to_as(v, asid);
> > @@ -140,6 +149,14 @@ static int vhost_vdpa_remove_as(struct vhost_vdpa *v, u32 asid)
> >
> >          hlist_del(&as->hash_link);
> >          vhost_vdpa_iotlb_unmap(v, &as->iotlb, 0ULL, 0ULL - 1, asid);
> > +       /*
> > +        * Devices with on-chip IOMMU need to restore iotlb
> > +        * to 1:1 identity mapping before vhost-vdpa is going
> > +        * to be removed and detached from the device. Give
> > +        * them a chance to do so, as this cannot be done
> > +        * efficiently via the whole-range unmap call above.
> > +        */
> Same question as before, if 1:1 is restored and the userspace doesn't
> do any IOTLB updating. It looks like a security issue? (Assuming IOVA
> is PA)
This is already flawed independent of this series. It was introduced 
from the two commits I referenced earlier in the other thread. Today 
userspace is already able to do so with device reset and don't do any 
IOTLB update. This series don't get it worse nor make it better.

FWIW as said earlier, to address this security issue properly we 
probably should set up 1:1 DMA mapping in virtio_vdpa_probe() on demand, 
and tears it down at virtio_vdpa_release_dev(). Question is, was 
virtio-vdpa the only vdpa bus user that needs 1:1 DMA mapping, or it's 
the other way around that vhost-vdpa is the only exception among all 
vdpa bus drivers that don't want to start with 1:1 by default. This 
would help parent vdpa implementation for what kind of mapping it should 
start with upon creation.

Regards,
-Siwei



> Thanks
>
> > +       vhost_vdpa_reset_map(v, asid);
> >          kfree(as);
> >
> >          return 0;
> > --
> > 1.8.3.1