Virtualization - Oct 2021 - [PATCH 0/5] iommu/virtio: Add identity domains

On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin
wrote:
> > From: Jean-Philippe Brucker <jean-philippe at linaro.org>
> > Sent: Wednesday, October 13, 2021 8:11 PM
> > 
> > Support identity domains, allowing to only enable IOMMU protection for
a
> > subset of endpoints (those assigned to userspace, for example). Users
> > may enable identity domains at compile time
> > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > (iommu.passthrough=1) or
> > runtime (/sys/kernel/iommu_groups/*/type = identity).
> 
> Do we want to use consistent terms between spec (bypass domain) 
> and code (identity domain)? 
I don't think we have to. Linux uses "identity" domains and
"passthrough"
IOMMU. The old virtio-iommu feature was "bypass" so we should keep
that
for the new one, to be consistent. And then I've used "bypass" for
domains
as well, in the spec, because it would look strange to use a different
term for the same concept. I find that it sort of falls into place: Linux'
identity domains can be implemented either with bypass or identity-mapped
virtio-iommu domains.
> > 
> > Patches 1-2 support identity domains using the optional
> > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet in
the
> > spec, see [1] for the latest proposal.
> > 
> > Patches 3-5 add a fallback to identity mappings, when the feature is
not
> > supported.
> > 
> > Note that this series doesn't touch the global bypass bit added by
> > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > should
> > be attached to a domain, so global bypass isn't in use after
endpoints
> 
> I saw a concept of deferred attach in iommu core. See iommu_is_
> attach_deferred(). Currently this is vendor specific and I haven't
> looked into the exact reason why some vendor sets it now. Just
> be curious whether the same reason might be applied to virtio-iommu.
> 
> > are probed. Before that, the global bypass policy is decided by the
> > hypervisor and firmware. So I don't think Linux needs to touch the
> 
> This reminds me one thing. The spec says that the global bypass
> bit is sticky and not affected by reset.
The spec talks about *device* reset, triggered by software writing 0 to
the status register, but it doesn't mention system reset. Would be good to
clarify that. Something like:

    If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
    initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
    NOT change on device reset, but SHOULD be restored to its initial
    value on system reset.
> This implies that in the case
> of rebooting the VM into a different OS, the previous OS actually
> has the right to override this setting for the next OS. Is it a right
> design? Even the firmware itself is unable to identify the original
> setting enforced by the hypervisor after reboot. I feel the hypervisor
> setting should be recovered after reset since it reflects the 
> security measure enforced by the virtual platform?
So I think clarifying system reset should address your questions.
I believe we should leave bypass sticky across device reset, so a FW->OS
transition, where the OS resets the device, does not open a vulnerability
(if bypass was enabled at boot and then left disabled by FW.)

It's still a good idea for the OS to restore on shutdown the bypass value
it was booted with. So it can kexec into an OS that doesn't support
virtio-iommu, for example.

Thanks,
Jean

On Mon, Oct 18, 2021 at 04:23:41PM +0100, Jean-Philippe Brucker
wrote:
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker <jean-philippe at linaro.org>
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > > 
> > > Support identity domains, allowing to only enable IOMMU
protection for a
> > > subset of endpoints (those assigned to userspace, for example).
Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> > 
> > Do we want to use consistent terms between spec (bypass domain) 
> > and code (identity domain)? 
> 
> I don't think we have to. Linux uses "identity" domains and
"passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should
keep that
> for the new one, to be consistent. And then I've used
"bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place:
Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.
> 
> > > 
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet
in the
> > > spec, see [1] for the latest proposal.
> > > 
> > > Patches 3-5 add a fallback to identity mappings, when the feature
is not
> > > supported.
> > > 
> > > Note that this series doesn't touch the global bypass bit
added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after
endpoints
> > 
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> > 
> > > are probed. Before that, the global bypass policy is decided by
the
> > > hypervisor and firmware. So I don't think Linux needs to
touch the
> > 
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
> 
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good
to
> clarify that. Something like:
> 
>     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
>     NOT change on device reset, but SHOULD be restored to its initial
>     value on system reset.
> 
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the 
> > security measure enforced by the virtual platform?
> 
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
> 
> It's still a good idea for the OS to restore on shutdown the bypass
value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
> 
> Thanks,
> Jean
Is this stickiness really important? Can't this be addressed just by
hypervisor disabling bypass at boot?

-- 
MST

> From: Jean-Philippe Brucker <jean-philippe at linaro.org>
> Sent: Monday, October 18, 2021 11:24 PM
> 
> On Thu, Oct 14, 2021 at 03:00:38AM +0000, Tian, Kevin wrote:
> > > From: Jean-Philippe Brucker <jean-philippe at linaro.org>
> > > Sent: Wednesday, October 13, 2021 8:11 PM
> > >
> > > Support identity domains, allowing to only enable IOMMU
protection for
> a
> > > subset of endpoints (those assigned to userspace, for example).
Users
> > > may enable identity domains at compile time
> > > (CONFIG_IOMMU_DEFAULT_PASSTHROUGH), boot time
> > > (iommu.passthrough=1) or
> > > runtime (/sys/kernel/iommu_groups/*/type = identity).
> >
> > Do we want to use consistent terms between spec (bypass domain)
> > and code (identity domain)?
> 
> I don't think we have to. Linux uses "identity" domains and
"passthrough"
> IOMMU. The old virtio-iommu feature was "bypass" so we should
keep that
> for the new one, to be consistent. And then I've used
"bypass" for domains
> as well, in the spec, because it would look strange to use a different
> term for the same concept. I find that it sort of falls into place:
Linux'
> identity domains can be implemented either with bypass or identity-mapped
> virtio-iommu domains.
make sense.
> 
> > >
> > > Patches 1-2 support identity domains using the optional
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG feature. The feature bit is not yet
in
> the
> > > spec, see [1] for the latest proposal.
> > >
> > > Patches 3-5 add a fallback to identity mappings, when the feature
is not
> > > supported.
> > >
> > > Note that this series doesn't touch the global bypass bit
added by
> > > VIRTIO_IOMMU_F_BYPASS_CONFIG. All endpoints managed by the
> IOMMU
> > > should
> > > be attached to a domain, so global bypass isn't in use after
endpoints
> >
> > I saw a concept of deferred attach in iommu core. See iommu_is_
> > attach_deferred(). Currently this is vendor specific and I haven't
> > looked into the exact reason why some vendor sets it now. Just
> > be curious whether the same reason might be applied to virtio-iommu.
> >
> > > are probed. Before that, the global bypass policy is decided by
the
> > > hypervisor and firmware. So I don't think Linux needs to
touch the
> >
> > This reminds me one thing. The spec says that the global bypass
> > bit is sticky and not affected by reset.
> 
> The spec talks about *device* reset, triggered by software writing 0 to
> the status register, but it doesn't mention system reset. Would be good
to
> clarify that. Something like:
> 
>     If the device offers the VIRTIO_IOMMU_F_BYPASS_CONFIG feature, it MAY
>     initialize the \field{bypass} field to 1. Field \field{bypass} SHOULD
>     NOT change on device reset, but SHOULD be restored to its initial
>     value on system reset.
looks good to me
> 
> > This implies that in the case
> > of rebooting the VM into a different OS, the previous OS actually
> > has the right to override this setting for the next OS. Is it a right
> > design? Even the firmware itself is unable to identify the original
> > setting enforced by the hypervisor after reboot. I feel the hypervisor
> > setting should be recovered after reset since it reflects the
> > security measure enforced by the virtual platform?
> 
> So I think clarifying system reset should address your questions.
> I believe we should leave bypass sticky across device reset, so a FW->OS
> transition, where the OS resets the device, does not open a vulnerability
> (if bypass was enabled at boot and then left disabled by FW.)
> 
> It's still a good idea for the OS to restore on shutdown the bypass
value
> it was booted with. So it can kexec into an OS that doesn't support
> virtio-iommu, for example.
> 
Thanks
Kevin