Using packed virtqueues in Confidential VMs

Using packed virtqueues in Confidential VMs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1819 bytes --]

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?

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. 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.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Using packed virtqueues in Confidential VMs

[Michael S. Tsirkin]

On Thu, Nov 16, 2023 at 03:02:45PM -0500, Stefan Hajnoczi wrote:
> 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?
> 
> 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. Software devices (QEMU,
> vhost kernel, or vhost-user) usually have full access to guest RAM.

Not with encrypted memory.

> 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.
> 
> Stefan

RE: Using packed virtqueues in Confidential VMs

[Reshetova, Elena]

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. 

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

Re: Using packed virtqueues in Confidential VMs

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 5032 bytes --]

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. 
> 
> 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? 

Correct.

> 
> 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. 

I looked at the split virtqueue layout code and noticed
vring_unmap_one_split_indirect() does the same thing. So packed and
split virtqueues are the same with respect to loading descriptor fields
from memory that devices without an IOMMU may have overwritten.

The impact of letting an attacker choose most of the arguments to
dma_unmap_page() is not clear to me. There is swiotlb state associated
with every mapping. I haven't read the swiotlb code, so I'm not sure how
robust it is against invalid inputs.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Using packed virtqueues in Confidential VMs

[Michael S. Tsirkin]

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

RE: Using packed virtqueues in Confidential VMs

[Reshetova, Elena]

> 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.

Right, the above was only for non-speculative things.
For speculation, I have worked a while ago
with smatch maintainer to create a new smatch pattern
that is able to detect the speculation (spectre v1 style) issues overall
in the whole kernel for host <--> guest attack surface
(similarly as it does for userspace <--> kernel boundary). It covers virtio also 
(module my mistakes or limits on function pointer propagation in smatch). 

Here are the patterns in smatch: 
https://repo.or.cz/smatch.git/blob/045d29f90c4ab21c374ff587b856f3c30368750f:/smatch_kernel_host_data.c
https://repo.or.cz/smatch.git/blob/045d29f90c4ab21c374ff587b856f3c30368750f:/smatch_points_to_host_data.c

However, due to lack of resources at that time we have not been able to investigate
the reported issues and nothing has been done in the whole kernel about this. 
If you are interested to do this exercise for virtio, I will be happy to work
with you on this. I think we can first verify that it reports everything it needs
to cover virtio fully/correctly and then do a review on where it would make sense to 
insert the speculation barriers. 

> 
> >
> > 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/driver
> s/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.

Makes sense. We didn’t know you have done audit on this, so this is then covered, 
but ideally it would have to be validated with fuzzing on this mode also. 

This brings me to another point that I wanted to ask: when you did the
hardening audit, what did you consider as points of untrusted inputs? 
Is it just attributes of virtio-related structures (queues, etc) exposed to host
or did you also look at pci config space that virtio uses at attack vector? 

> 
> 
> > 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).

Yes, in this case we don’t have race/double fetch issues, but the main issue
seems to remain. 

Best Regards,
Elena