| Message ID | 20220304162344.539030-1-pasic@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/1] virtio: fix feature negotiation for ACCESS_PLATFORM | expand |
On Fri, Mar 04, 2022 at 05:23:44PM +0100, Halil Pasic 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. > > 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") I tried to apply this on top of virtio: fix the condition for iommu_platform not supported and it fails. Can you rebase this on top of my tree pls? > --- > v2 -> v2: > * Change comment: reflect that this is not about the verify > but also about the device features as seen by the driver (Connie) > 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 | 21 ++++++++++++++------- > 1 file changed, 14 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index fbf0dd14b8..d7ec023adf 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -78,16 +78,23 @@ 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); > + /* > + * Present IOMMU_PLATFORM to the driver iff iommu_plattform=on and > + * device operational. If the driver does not accept IOMMU_PLATFORM > + * we fail the device. > + */ > 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; > } > } > > > base-commit: 01d1ba29d05b992321d9941e8151c52c6845ce3c > -- > 2.32.0
On Sun, 6 Mar 2022 05:15:20 -0500 "Michael S. Tsirkin" <mst@redhat.com> wrote: > I tried to apply this on top of > virtio: fix the condition for iommu_platform not supported > and it fails. > Can you rebase this on top of my tree pls? Will do right away! BTW I don't think your tree is mentioned in the MAINTAINERS file. I think, it would probably make sense to add it. Regards, Halil
diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c index fbf0dd14b8..d7ec023adf 100644 --- a/hw/virtio/virtio-bus.c +++ b/hw/virtio/virtio-bus.c @@ -78,16 +78,23 @@ 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); + /* + * Present IOMMU_PLATFORM to the driver iff iommu_plattform=on and + * device operational. If the driver does not accept IOMMU_PLATFORM + * we fail the device. + */ 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; } }
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. 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") --- v2 -> v2: * Change comment: reflect that this is not about the verify but also about the device features as seen by the driver (Connie) 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 | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) base-commit: 01d1ba29d05b992321d9941e8151c52c6845ce3c