Re: [PATCH] virtio-iommu: Default to bypass during boot

[Jean-Philippe Brucker <jean-philippe@linaro.org>]

On Fri, Feb 26, 2021 at 08:11:41AM +0000, Tian, Kevin wrote:
> > 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. 

Yes in hindsight the polarity should probably have been reversed. Clearly
I could have put more thoughts into this. When writing the spec it seemed
natural in terms of security to disallow accesses until software
explicitly enables bypass.

In the linked discussion, which happened after the initial spec was
accepted, we concluded that QEMU should be boot-bypass by default and the
spec ought to be clarified to explicitly allow this, I just didn't take
time to work on it until now.

> In concept when a IOMMU device is uninitialized or reset, it essentially means 
> no protection in place thus a bypass behavior. 

Not necessarily, the SMMU leaves that choice to the implementation (but it
does have the right polarity 0 == bypass). The SMMU_GBPA register defines
the bypass behavior. Implementations can tie it to 0 or 1 to define the
default behavior of abort or bypass. Then software sets it to 0 or 1 to
select the runtime policy. Ideally I'd like a similar mechanism for
virtio-iommu, because it's sticky across reset - no vulnerability window
when firmware hands over the IOMMU to the OS.

Thanks,
Jean