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

[Yongji Xie]

On Fri, May 14, 2021 at 7:17 PM Stefan Hajnoczi <stefanha@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxxx> 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.
>

Oh, that's great. Thank you!

I also did some audit work these days and will send a new version for
reviewing next Monday.

Thanks,
Yongji