Re: [PATCH for-5.0 v11 08/20] virtio-iommu: Implement translate

[Jean-Philippe Brucker]

On Thu, Dec 19, 2019 at 04:09:47PM +0100, Auger Eric wrote:
> >>>>>> @@ -412,19 +412,80 @@ static IOMMUTLBEntry 
> >>>>>> virtio_iommu_translate(IOMMUMemoryRegion *mr, hwaddr addr,
> >>>>>>                                              int iommu_idx)
> >>>>>>  {
> >>>>>>      IOMMUDevice *sdev = container_of(mr, IOMMUDevice, iommu_mr);
> >>>>>> +    viommu_interval interval, *mapping_key;
> >>>>>> +    viommu_mapping *mapping_value;
> >>>>>> +    VirtIOIOMMU *s = sdev->viommu;
> >>>>>> +    viommu_endpoint *ep;
> >>>>>> +    bool bypass_allowed;
> >>>>>>      uint32_t sid;
> >>>>>> +    bool found;
> >>>>>> +
> >>>>>> +    interval.low = addr;
> >>>>>> +    interval.high = addr + 1;
> >>>>>>  
> >>>>>>      IOMMUTLBEntry entry = {
> >>>>>>          .target_as = &address_space_memory,
> >>>>>>          .iova = addr,
> >>>>>>          .translated_addr = addr,
> >>>>>> -        .addr_mask = ~(hwaddr)0,
> >>>>>> +        .addr_mask = (1 << ctz32(s->config.page_size_mask)) - 1,
> >>>>>>          .perm = IOMMU_NONE,
> >>>>>>      };
> >>>>>>  
> >>>>>> +    bypass_allowed = virtio_has_feature(s->acked_features,
> >>>>>> +                                        VIRTIO_IOMMU_F_BYPASS);
> >>>>>> +
> >>>>>
> >>>>> Would it be easier to check bypass_allowed here once and then drop the
> >>>>> latter [1] and [2] check?
> >>>> bypass_allowed does not mean you systematically bypass. You bypass if
> >>>> the SID is unknown or if the device is not attached to any domain.
> >>>> Otherwise you translate. But maybe I miss your point.
> >>>
> >>> Ah ok, then could I ask how will this VIRTIO_IOMMU_F_BYPASS be used?
> >>> For example, I think VT-d defines passthrough in a totally different
> >>> way in that the PT mark will be stored in the per-device context
> >>> entries, then we can allow a specific device to be pass-through when
> >>> doing DMA.  That information is explicit (e.g., unknown SID will
> >>> always fail the DMA), and per-device.
> >>>
> >>> Here do you mean that you just don't put a device into any domain to
> >>> show it wants to use PT?  Then I'm not sure how do you identify
> >>> whether this is a legal PT or a malicious device (e.g., an unknown
> >>> device that even does not have any driver bound to it, which will also
> >>> satisfy "unknown SID" and "not attached to any domain", iiuc).
> >>
> >> The virtio-iommu spec currently says:
> >>
> >> "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.
> >> "
> >>
> >> I guess this can serve the purpose of devices doing early accesses,
> >> before the guest OS gets the hand and maps them?
> > 
> > OK, so there's no global enablement knob for virtio-iommu? Hmm... Then:

There is at the virtio transport level: the driver sets status to
FEATURES_OK once it accepted the feature bits, and to DRIVER_OK once its
fully operational. The virtio-iommu spec says:

  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 before features negotiation, there is no access. Afterwards it depends
if the VIRTIO_IOMMU_F_BYPASS has been accepted by the driver.

> well this is a global knob. If this is bot negotiated any unmapped
> device can PT.
> 
> My assumption above must be wrong as this is a negotiated feature so
> anyway the virtio-iommu driver should be involved.
> 
> I don't really remember the rationale of the feature bit tbh.

I don't remember writing down a rationale for this bit, it was in the very
first version (I think someone suggested it during the initial internal
discussion) and I didn't remove it afterwards because it seems useful:

Say a guest only wants to use the vIOMMU for userspace assignment and
wants all other endpoints to bypass translation, which is our primary
use-case. In other words booting Linux with iommu.passthrough=1. It can
either create an identity domain for each endpoint (one MAP request with
VA==PA) or it can set the VIRTIO_IOMMU_F_BYPASS bit. The device-side
implementation should be more efficient with the latter, since you don't
need to lookup the domain + address space for each access.

> In "[virtio-dev] RE: [RFC] virtio-iommu version 0.4 " Jean discussed
> that with Kevein. Sorry I cannot find the link.
> 
> " If the endpoint is not attached to any address space,
> then the device MAY abort the transaction."

Hmm, that was regarding a "bypass" reserved memory region, which isn't in
the current spec.

> Kevin> From definition of BYPASS, it's orthogonal to whether there is an
> address space attached, then should we still allow "May abort" behavior?
> 
> Jean> The behavior is left as an implementation choice, and I'm not sure
> it's worth enforcing in the architecture. If the endpoint isn't attached
> to any domain then (unless VIRTIO_IOMMU_F_BYPASS is negotiated), it
> isn't necessarily able to do DMA at all. The virtio-iommu device may
> setup DMA mastering lazily, in which case any DMA transaction would
> abort, or have setup DMA already, in which case the endpoint can access
> MEM_T_BYPASS regions.
> 
> Hopefully Jean will remember and comment on this.
> 
> Thanks
> 
> Eric
> 
> > 
> >   - This flag is a must for all virtio-iommu emulation, right?
> >     (otherwise I can't see how system bootstraps..)

What do you mean by system bootstrap?

One thing I've been wondering, and may be related, is how to handle a
bootloader that wants to read for example an initrd from a virtio-block
device that's behind the IOMMU. Either we allow the device to let any DMA
bypass the device until FEATURES_OK, which is a source of vulnerabilities
[1], or we have to implement some support for the virtio-iommu in the
BIOS. Again the F_BYPASS bit would help for this, since all the BIOS has
to do is set it on boot. However, F_BYPASS is optional, and more complex
support is needed for setting up identity mappings.

[1] See "IOMMU protection against I/O attacks: a vulnerability and a proof
of concept" by Morgan et al, where a malicious device bypassing the IOMMU
overwrites the IOMMU configuration as it is being created by the OS.
Arguably we're not too concerned about malicious devices at the moment,
but I'm not comfortable relaxing this.

> >   - Should this flag be gone right after OS starts (otherwise I think
> >     we still have the issue that any malicious device can be seen as
> >     in PT mode as default)?  How is that done?

Yes bypass mode assumes that devices and drivers aren't malicious, and the
IOMMU is only used for things like assigning devices to guest userspace,
or having large contiguous DMA buffers.

Thanks,
Jean