Re: [RFC PATCH V2 0/7] Do not read from descripto ring

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3693 bytes --]

On Fri, May 14, 2021 at 03:29:20PM +0800, Jason Wang wrote:
> On Fri, May 14, 2021 at 12:27 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >
> > On Fri, Apr 23, 2021 at 04:09:35PM +0800, Jason Wang wrote:
> > > Sometimes, the driver doesn't trust the device. This is usually
> > > happens for the encrtpyed VM or VDUSE[1].
> >
> > Thanks for doing this.
> >
> > Can you describe the overall memory safety model that virtio drivers
> > must follow?
> 
> My understanding is that, basically the driver should not trust the
> device (since the driver doesn't know what kind of device that it
> tries to drive)
> 
> 1) For any read only metadata (required at the spec level) which is
> mapped as coherent, driver should not depend on the metadata that is
> stored in a place that could be wrote by the device. This is what this
> series tries to achieve.
> 2) For other metadata that is produced by the device, need to make
> sure there's no malicious device triggered behavior, this is somehow
> similar to what vhost did. No DOS, loop, kernel bug and other stuffs.
> 3) swiotb is a must to enforce memory access isolation. (VDUSE or encrypted VM)
> 
> > For example:
> >
> > - Driver-to-device buffers must be on dedicated pages to avoid
> >   information leaks.
> 
> It looks to me if swiotlb is used, we don't need this since the
> bouncing is not done at byte not page.
> 
> But if swiotlb is not used, we need to enforce this.
> 
> >
> > - Driver-to-device buffers must be on dedicated pages to avoid memory
> >   corruption.
> 
> Similar to the above.
> 
> >
> > When I say "pages" I guess it's the IOMMU page size that matters?
> >
> 
> And the IOTLB page size.
> 
> > What is the memory access granularity of VDUSE?
> 
> It has an swiotlb, but the access and bouncing is done per byte.
> 
> >
> > I'm asking these questions because there is driver code that exposes
> > kernel memory to the device and I'm not sure it's safe. For example:
> >
> >   static int virtblk_add_req(struct virtqueue *vq, struct virtblk_req *vbr,
> >                   struct scatterlist *data_sg, bool have_data)
> >   {
> >           struct scatterlist hdr, status, *sgs[3];
> >           unsigned int num_out = 0, num_in = 0;
> >
> >           sg_init_one(&hdr, &vbr->out_hdr, sizeof(vbr->out_hdr));
> >                             ^^^^^^^^^^^^^
> >           sgs[num_out++] = &hdr;
> >
> >           if (have_data) {
> >                   if (vbr->out_hdr.type & cpu_to_virtio32(vq->vdev, VIRTIO_BLK_T_OUT))
> >                           sgs[num_out++] = data_sg;
> >                   else
> >                           sgs[num_out + num_in++] = data_sg;
> >           }
> >
> >           sg_init_one(&status, &vbr->status, sizeof(vbr->status));
> >                                ^^^^^^^^^^^^
> >           sgs[num_out + num_in++] = &status;
> >
> >           return virtqueue_add_sgs(vq, sgs, num_out, num_in, vbr, GFP_ATOMIC);
> >   }
> >
> > I guess the drivers don't need to be modified as long as swiotlb is used
> > to bounce the buffers through "insecure" memory so that the memory
> > surrounding the buffers is not exposed?
> 
> Yes, swiotlb won't bounce the whole page. So I think it's safe.

Thanks Jason and Yongji Xie for clarifying. Seems like swiotlb or a
similar mechanism can handle byte-granularity isolation so the drivers
not need to worry about information leaks or memory corruption outside
the mapped byte range.

We still need to audit virtio guest drivers to ensure they don't trust
data that can be modified by the device. I will look at virtio-blk and
virtio-fs next week.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]