From: "Michael S. Tsirkin" <mst@redhat.com>
To: "Reshetova, Elena" <elena.reshetova@intel.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>,
"virtio-dev@lists.oasis-open.org"
<virtio-dev@lists.oasis-open.org>,
"virtualization@lists.linux.dev" <virtualization@lists.linux.dev>
Subject: [virtio-dev] Re: Using packed virtqueues in Confidential VMs
Date: Mon, 20 Nov 2023 07:29:39 -0500 [thread overview]
Message-ID: <20231120072623-mutt-send-email-mst@kernel.org> (raw)
In-Reply-To: <DM8PR11MB5750765B1310C904A8B1B00BE7B4A@DM8PR11MB5750.namprd11.prod.outlook.com>
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@redhat.com>
> > Sent: Thursday, November 16, 2023 10:03 PM
> > To: Reshetova, Elena <elena.reshetova@intel.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>; virtio-dev@lists.oasis-open.org;
> > virtualization@lists.linux.dev
> > 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
---------------------------------------------------------------------
To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
prev parent reply other threads:[~2023-11-20 12:29 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-16 20:02 [virtio-dev] Using packed virtqueues in Confidential VMs Stefan Hajnoczi
2023-11-16 22:26 ` [virtio-dev] " Michael S. Tsirkin
[not found] ` <DM8PR11MB5750765B1310C904A8B1B00BE7B4A@DM8PR11MB5750.namprd11.prod.outlook.com>
2023-11-20 11:02 ` Stefan Hajnoczi
2023-11-20 12:29 ` Michael S. Tsirkin [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231120072623-mutt-send-email-mst@kernel.org \
--to=mst@redhat.com \
--cc=elena.reshetova@intel.com \
--cc=stefanha@redhat.com \
--cc=virtio-dev@lists.oasis-open.org \
--cc=virtualization@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).