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

[Yongji Xie]

On Fri, May 14, 2021 at 7:36 PM Michael S. Tsirkin <mst@xxxxxxxxxx> wrote:
>
> On Fri, May 14, 2021 at 07:27:22PM +0800, Yongji Xie wrote:
> > 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
>
> Doing it in a way that won't hurt performance for simple
> configs that trust the device is a challenge though.
> Pls take a look at the discussion with Christoph for some ideas
> on how to do this.
>

I see. Thanks for the reminder.

Thanks,
Yongji