On Mon, Nov 20, 2023 at 10:13:15AM +0000, Reshetova, Elena wrote: > 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. However, both with packed and split I don't think speculation barriers have been added and they are likely to be needed. I wonder whether your fuzzing included attempts to force spectre like leaks based on speculation execution. > > 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. packed has R/W descriptors. This means however that data is copied over from descriptor and validated before use. > 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? > I think this whole table is copied to swiotlb (this is what DMA_TO_DEVICE AFAIK). > 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
