Hi Stefan, Thank you for following up on this! Please find my comments inline. > -----Original Message----- > From: Stefan Hajnoczi <stefanha@xxxxxxxxxx> > Sent: Thursday, November 16, 2023 10:03 PM > To: Reshetova, Elena <elena.reshetova@xxxxxxxxx> > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>; virtio-dev@xxxxxxxxxxxxxxxxxxxx; > virtualization@xxxxxxxxxxxxxxx > Subject: Using packed virtqueues in Confidential VMs > > Hi Elena, > You raised concerns about using packed virtqueues with untrusted devices at > Linux Plumbers Conference. I reviewed the specification and did not find > fundamental issues that would preclude the use of packed virtqueues in > untrusted devices. Do you have more information about issues with packed > virtqueues? First of all a bit of clarification: our overall logic for making our first reference release of Linux intel tdx stacks [1] was to enable only minimal required functionality and this also applied to numerous modes that virtio provided. Because for each enabled functionality we would have to do a code audit and a proper fuzzing setup and all of this requires resources. The choice of split queue was a natural first step since it is the most straightforward to understand (at least it was for us, bare in mind we are not experts in virtio as you are) and the fact that there was work already done ([2] and other patches) to harden the descriptors for split queue. [1] https://github.com/intel/tdx-tools [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/virtio?h=v6.6-rc4&id=72b5e8958738aaa453db5149e6ca3bcf416023b9 I remember looking at the packed queue long ago and noticing that at least some descriptor fields are under device control and I wasn’t sure why the similar hardening was not done here as for the split case. However, we had many issues to handle in past, and since we didn’t need the packed queue, we never went to investigate this further. It is also possible that we simply misunderstood the code at that point. > > I also reviewed Linux's virtio_ring.c to look for implementation issues. One > thing I noticed was that detach_buf_packed -> vring_unmap_desc_packed trusts > the fields of indirect descriptors that have been mapped to the device: > > flags = le16_to_cpu(desc->flags); > > dma_unmap_page(vring_dma_dev(vq), > le64_to_cpu(desc->addr), > le32_to_cpu(desc->len), > (flags & VRING_DESC_F_WRITE) ? > DMA_FROM_DEVICE : DMA_TO_DEVICE); > > This could be problematic if the device is able to modify indirect descriptors. > However, the indirect descriptor table is mapped with DMA_TO_DEVICE: > > addr = vring_map_single(vq, desc, > total_sg * sizeof(struct vring_packed_desc), > DMA_TO_DEVICE); > > There is no problem when there is an enforcing IOMMU that maps the page with > read-only permissions but that's not always the case. We don’t use IOMMU at the moment for the confidential guest, since we don’t have to (memory is encrypted/protected) and only explicitly shared pages are available for the host/devices to modify. Do I understand it correctly that in our case the indirect descriptor table will end up mapped shared for this mode to work and then there is no protection? Software devices (QEMU, > vhost kernel, or vhost-user) usually have full access to guest RAM. They can > cause dma_unmap_page() to be invoked with arguments of their choice (except > for > the first argument) by modifying indirect descriptors. > > I am not sure if this poses a danger since software devices already have access > to guest RAM, but I think this code is risky. It would be safer for the driver > to stash away the arguments needed for dma_unmap_page() in memory that is > not > mapped to the device. > > Other than that, I didn't find any issues with the packed virtqueue > implementation. Thank you for looking into this! Even if we didn’t need the packed queue, I am sure other deployments might need it and it would be the best for virtio to provide all modes that are secure. Best Regards, Elena. > > Stefan
