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

[Jason Wang]

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

>
> Stefan