include/hw/virtio/virtio-iommu.h | 1 + hw/virtio/virtio-iommu.c | 23 +++++++++++++++++++++-- 2 files changed, 22 insertions(+), 2 deletions(-)
Currently the virtio-iommu device must be programmed before it allows
DMA from any PCI device. This can make the VM entirely unusable when a
virtio-iommu driver isn't present, for example in a bootloader that
loads the OS from storage.
Similarly to the other vIOMMU implementations, default to DMA bypassing
the IOMMU during boot. Add a "boot-bypass" option that lets users change
this behavior.
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
include/hw/virtio/virtio-iommu.h | 1 +
hw/virtio/virtio-iommu.c | 23 +++++++++++++++++++++--
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h
index 273e35c04bc..4c66989ca4e 100644
--- a/include/hw/virtio/virtio-iommu.h
+++ b/include/hw/virtio/virtio-iommu.h
@@ -58,6 +58,7 @@ struct VirtIOIOMMU {
GTree *domains;
QemuMutex mutex;
GTree *endpoints;
+ bool boot_bypass;
};
#endif
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index c2883a2f6c8..cd08dc39eca 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -689,6 +689,25 @@ static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason,
}
+static bool virtio_iommu_bypass_is_allowed(VirtIOIOMMU *s)
+{
+ VirtIODevice *vdev = &s->parent_obj;
+
+ /*
+ * Allow bypass if:
+ * - boot_bypass is enabled and the BYPASS feature hasn't yet been
+ * acknowledged.
+ * - the BYPASS feature has been negotiated.
+ */
+ if (s->boot_bypass && !(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK)) {
+ return true;
+ }
+ if (virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS)) {
+ return true;
+ }
+ return false;
+}
+
static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
IOMMUAccessFlags flag,
int iommu_idx)
@@ -715,8 +734,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
.perm = IOMMU_NONE,
};
- bypass_allowed = virtio_vdev_has_feature(&s->parent_obj,
- VIRTIO_IOMMU_F_BYPASS);
+ bypass_allowed = virtio_iommu_bypass_is_allowed(s);
sid = virtio_iommu_get_bdf(sdev);
@@ -1156,6 +1174,7 @@ static const VMStateDescription vmstate_virtio_iommu = {
static Property virtio_iommu_properties[] = {
DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, "PCI", PCIBus *),
+ DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true),
DEFINE_PROP_END_OF_LIST(),
};
--
2.30.0
On Thu, Feb 18, 2021 at 11:59:30AM +0100, Jean-Philippe Brucker wrote: > Currently the virtio-iommu device must be programmed before it allows > DMA from any PCI device. This can make the VM entirely unusable when a > virtio-iommu driver isn't present, for example in a bootloader that > loads the OS from storage. > > Similarly to the other vIOMMU implementations, default to DMA bypassing > the IOMMU during boot. Add a "boot-bypass" option that lets users change > this behavior. > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> wouldn't this be a spec violation? When the device is reset, endpoints are not attached to any domain. If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from unattached endpoints are allowed and translated by the IOMMU using the identity function. If the feature is not negotiated, any memory access from an unattached endpoint fails. Upon attaching an endpoint in bypass mode to a new domain, any memory access from the endpoint fails, since the domain does not contain any mapping. Maybe it's not too late to change the spec here - want to try sending a spec patch? > --- > include/hw/virtio/virtio-iommu.h | 1 + > hw/virtio/virtio-iommu.c | 23 +++++++++++++++++++++-- > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/include/hw/virtio/virtio-iommu.h b/include/hw/virtio/virtio-iommu.h > index 273e35c04bc..4c66989ca4e 100644 > --- a/include/hw/virtio/virtio-iommu.h > +++ b/include/hw/virtio/virtio-iommu.h > @@ -58,6 +58,7 @@ struct VirtIOIOMMU { > GTree *domains; > QemuMutex mutex; > GTree *endpoints; > + bool boot_bypass; > }; > > #endif > diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c > index c2883a2f6c8..cd08dc39eca 100644 > --- a/hw/virtio/virtio-iommu.c > +++ b/hw/virtio/virtio-iommu.c > @@ -689,6 +689,25 @@ static void virtio_iommu_report_fault(VirtIOIOMMU *viommu, uint8_t reason, > > } > > +static bool virtio_iommu_bypass_is_allowed(VirtIOIOMMU *s) > +{ > + VirtIODevice *vdev = &s->parent_obj; > + > + /* > + * Allow bypass if: > + * - boot_bypass is enabled and the BYPASS feature hasn't yet been > + * acknowledged. > + * - the BYPASS feature has been negotiated. > + */ > + if (s->boot_bypass && !(vdev->status & VIRTIO_CONFIG_S_FEATURES_OK)) { > + return true; > + } > + if (virtio_vdev_has_feature(vdev, VIRTIO_IOMMU_F_BYPASS)) { > + return true; > + } > + return false; > +} > + > static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr, > IOMMUAccessFlags flag, > int iommu_idx) > @@ -715,8 +734,7 @@ static IOMMUTLBEntry virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr, > .perm = IOMMU_NONE, > }; > > - bypass_allowed = virtio_vdev_has_feature(&s->parent_obj, > - VIRTIO_IOMMU_F_BYPASS); > + bypass_allowed = virtio_iommu_bypass_is_allowed(s); > > sid = virtio_iommu_get_bdf(sdev); > > @@ -1156,6 +1174,7 @@ static const VMStateDescription vmstate_virtio_iommu = { > > static Property virtio_iommu_properties[] = { > DEFINE_PROP_LINK("primary-bus", VirtIOIOMMU, primary_bus, "PCI", PCIBus *), > + DEFINE_PROP_BOOL("boot-bypass", VirtIOIOMMU, boot_bypass, true), > DEFINE_PROP_END_OF_LIST(), > }; > > -- > 2.30.0
On Sun, Feb 21, 2021 at 06:45:18AM -0500, Michael S. Tsirkin wrote: > On Thu, Feb 18, 2021 at 11:59:30AM +0100, Jean-Philippe Brucker wrote: > > Currently the virtio-iommu device must be programmed before it allows > > DMA from any PCI device. This can make the VM entirely unusable when a > > virtio-iommu driver isn't present, for example in a bootloader that > > loads the OS from storage. > > > > Similarly to the other vIOMMU implementations, default to DMA bypassing > > the IOMMU during boot. Add a "boot-bypass" option that lets users change > > this behavior. > > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > wouldn't this be a spec violation? > > > When the device is reset, endpoints are not attached to any domain. > > If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from > unattached endpoints are allowed and translated by the IOMMU using the > identity function. If the feature is not negotiated, any memory access > from an unattached endpoint fails. Upon attaching an endpoint in > bypass mode to a new domain, any memory access from the endpoint fails, > since the domain does not contain any mapping. Right, but the device behavior regarding BYPASS is specified as: If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the device SHOULD NOT let endpoints access the guest-physical address space. So I figured that the spec could be read as "before feature negotiation it's undefined whether the IOMMU is bypassed or not" and so a device implementation can choose either (previously discussed at [1]). But it's a stretch, we should clarify this. Also, the OS might want to know whether DMA was bypassing the IOMMU before the device is initialized. If there are strong security requirements, then boot-bypass means the system was vulnerable to DMA attacks during boot. So I'd like to add a new feature bit for this, VIRTIO_IOMMU_F_BOOT_BYPASS, that tells whether DMA bypasses the IOMMU before feature negotiation. It's only an indicator, accepting the feature doesn't change anything. I'll send the spec change shortly. Thanks, Jean [1] https://lore.kernel.org/qemu-devel/20200109104032.GA937123@myrica/ > Maybe it's not too late to change the spec here - want to try sending > a spec patch?
> From: Qemu-devel <qemu-devel-bounces+kevin.tian=intel.com@nongnu.org> > On Behalf Of Jean-Philippe Brucker > > On Sun, Feb 21, 2021 at 06:45:18AM -0500, Michael S. Tsirkin wrote: > > On Thu, Feb 18, 2021 at 11:59:30AM +0100, Jean-Philippe Brucker wrote: > > > Currently the virtio-iommu device must be programmed before it allows > > > DMA from any PCI device. This can make the VM entirely unusable when > a > > > virtio-iommu driver isn't present, for example in a bootloader that > > > loads the OS from storage. > > > > > > Similarly to the other vIOMMU implementations, default to DMA > bypassing > > > the IOMMU during boot. Add a "boot-bypass" option that lets users > change > > > this behavior. > > > > > > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org> > > > > wouldn't this be a spec violation? > > > > > > When the device is reset, endpoints are not attached to any domain. > > > > If the VIRTIO_IOMMU_F_BYPASS feature is negotiated, all accesses from > > unattached endpoints are allowed and translated by the IOMMU using the > > identity function. If the feature is not negotiated, any memory access > > from an unattached endpoint fails. Upon attaching an endpoint in > > bypass mode to a new domain, any memory access from the endpoint fails, > > since the domain does not contain any mapping. > > Right, but the device behavior regarding BYPASS is specified as: > > If the driver does not accept the VIRTIO_IOMMU_F_BYPASS feature, the > device SHOULD NOT let endpoints access the guest-physical address space. > > So I figured that the spec could be read as "before feature negotiation > it's undefined whether the IOMMU is bypassed or not" and so a device > implementation can choose either (previously discussed at [1]). But it's a > stretch, we should clarify this. > > Also, the OS might want to know whether DMA was bypassing the IOMMU > before > the device is initialized. If there are strong security requirements, then > boot-bypass means the system was vulnerable to DMA attacks during boot. > > So I'd like to add a new feature bit for this, VIRTIO_IOMMU_F_BOOT_BYPASS, > that tells whether DMA bypasses the IOMMU before feature negotiation. It's > only an indicator, accepting the feature doesn't change anything. I'll > send the spec change shortly. > > Thanks, > Jean > > [1] https://lore.kernel.org/qemu- > devel/20200109104032.GA937123@myrica/ > I wonder why we didn't define the default behavior to bypass (to align with other vIOMMUs) in the first place and then have an option (e.g. VIRTIO_IOMMU_F_NOBYPASS) to override in case a stricter policy is required. In concept when a IOMMU device is uninitialized or reset, it essentially means no protection in place thus a bypass behavior. Thanks Kevin
© 2016 - 2024 Red Hat, Inc.