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

On Fri, May 14, 2021 at 07:27:22PM +0800, Yongji Xie wrote:

On Fri, May 14, 2021 at 7:17 PM Stefan Hajnoczi [off-list ref] wrote:

quoted

On Fri, May 14, 2021 at 03:29:20PM +0800, Jason Wang wrote:

quoted

On Fri, May 14, 2021 at 12:27 AM Stefan Hajnoczi [off-list ref] wrote:

quoted

On Fri, Apr 23, 2021 at 04:09:35PM +0800, Jason Wang wrote:

quoted

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)

quoted

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.

quoted

- Driver-to-device buffers must be on dedicated pages to avoid memory
  corruption.

Similar to the above.

quoted

When I say "pages" I guess it's the IOMMU page size that matters?

And the IOTLB page size.

quoted

What is the memory access granularity of VDUSE?

It has an swiotlb, but the access and bouncing is done per byte.

quoted

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.


-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization