[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM
From: |
Cornelia Huck |
Subject: |
Re: [PATCH 1/1] virtio: fix feature negotiation for ACCESS_PLATFORM |
Date: |
Wed, 09 Feb 2022 18:24:56 +0100 |
User-agent: |
Notmuch/0.34 (https://notmuchmail.org) |
On Wed, Feb 09 2022, Halil Pasic <pasic@linux.ibm.com> wrote:
> Unlike most virtio features ACCESS_PLATFORM is considered mandatory by
> QEMU, i.e. the driver must accept it if offered by the device. The
> virtio specification says that the driver SHOULD accept the
> ACCESS_PLATFORM feature if offered, and that the device MAY fail to
> operate if ACCESS_PLATFORM was offered but not negotiated.
Maybe add
(see the "{Driver,Device} Requirements: Reserved Feature Bits" sections
in the virtio spec)
?
>
> While a SHOULD ain't exactly a MUST, we are certainly allowed to fail
> the device when the driver fences ACCESS_PLATFORM. With commit
> 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM") we already made the
> decision to do so whenever the get_dma_as() callback is implemented (by
> the bus), which in practice means for the entirety of virtio-pci.
>
> That means, if the device needs to translate I/O addresses, then
> ACCESS_PLATFORM is mandatory. The aforementioned commit tells us in the
> commit message that this is for security reasons. More precisely if we
> were to allow a less then trusted driver (e.g. an user-space driver, or
> a nested guest) to make the device bypass the IOMMU by not negotiating
> ACCESS_PLATFORM, then the guest kernel would have no ability to
> control/police (by programming the IOMMU) what pieces of guest memory
> the driver may manipulate using the device. Which would break security
> assumptions within the guest.
>
> If ACCESS_PLATFORM is offered not because we want the device to utilize
> an IOMMU and do address translation, but because the device does not
> have access to the entire guest RAM, and needs the driver to grant
> access to the bits it needs access to (e.g. confidential guest support),
> we still require the guest to have the corresponding logic and to accept
> ACCESS_PLATFORM. If the driver does not accept ACCESS_PLATFORM, then
> things are bound to go wrong, and we may see failures much less graceful
> than failing the device because the driver didn't negotiate
> ACCESS_PLATFORM.
>
> So let us make ACCESS_PLATFORM mandatory for the driver regardless
> of whether the get_dma_as() callback is implemented or not.
>
> Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
> Fixes: 2943b53f68 ("virtio: force VIRTIO_F_IOMMU_PLATFORM")
>
> ---
>
> RFC -> v1:
> * Tweaked the commit message and fixed typos (Connie)
> * Added two sentences discussing the security implications (Michael)
>
> This patch is based on:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg866199.html
>
> During the review of "virtio: fix the condition for iommu_platform not
> supported" Daniel raised the question why do we "force IOMMU_PLATFORM"
> iff has_iommu && !!klass->get_dma_as. My answer to that was, that
> this logic ain't right.
>
> While at it I used the opportunity to re-organize the code a little
> and provide an explanatory comment.
> ---
> hw/virtio/virtio-bus.c | 17 ++++++++++-------
> 1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index fbf0dd14b8..359430eb1c 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -78,16 +78,19 @@ void virtio_bus_device_plugged(VirtIODevice *vdev, Error
> **errp)
> return;
> }
>
> - vdev_has_iommu = virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> - if (klass->get_dma_as != NULL && has_iommu) {
> + vdev->dma_as = &address_space_memory;
> + if (has_iommu) {
> + vdev_has_iommu = virtio_host_has_feature(vdev,
> VIRTIO_F_IOMMU_PLATFORM);
> + /* Fail FEATURE_OK if the device tries to drop IOMMU_PLATFORM */
I must admit that the more I stare at this code, the more confused I
get. We run this function during device realization, and the reason that
the feature bit might have gotten lost is that the ->get_features()
device callback dropped it. This happens before the driver is actually
involved; the check whether the *driver* dropped the feature is done
during feature validation, which is another code path. So what we do
here is failing device realization if a backend doesn't support
IOMMU_PLATFORM, isn't it?
> virtio_add_feature(&vdev->host_features, VIRTIO_F_IOMMU_PLATFORM);
> - vdev->dma_as = klass->get_dma_as(qbus->parent);
> - if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> - error_setg(errp,
> + if (klass->get_dma_as) {
> + vdev->dma_as = klass->get_dma_as(qbus->parent);
> + if (!vdev_has_iommu && vdev->dma_as != &address_space_memory) {
> + error_setg(errp,
> "iommu_platform=true is not supported by the device");
> + return;
> + }
> }
> - } else {
> - vdev->dma_as = &address_space_memory;
> }
> }
>