RE: [PATCH v4] IOMMU: make DMA containment of quarantined devices optional

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: 30 November 2020 11:59
> To: paul@xxxxxxx
> Cc: 'Andrew Cooper' <andrew.cooper3@xxxxxxxxxx>; 'Kevin Tian' 
> <kevin.tian@xxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4] IOMMU: make DMA containment of quarantined devices 
> optional
> 
> On 30.11.2020 11:45, Paul Durrant wrote:
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: 27 November 2020 16:46
> >>
> >> --- a/docs/misc/xen-command-line.pandoc
> >> +++ b/docs/misc/xen-command-line.pandoc
> >> @@ -1278,7 +1278,7 @@ detection of systems known to misbehave
> >>  > Default: `new` unless directed-EOI is supported
> >>
> >>  ### iommu
> >> -    = List of [ <bool>, verbose, debug, force, required, quarantine,
> >> +    = List of [ <bool>, verbose, debug, force, required, 
> >> quarantine[=scratch-page],
> >>                  sharept, intremap, intpost, crash-disable,
> >>                  snoop, qinval, igfx, amd-iommu-perdev-intremap,
> >>                  dom0-{passthrough,strict} ]
> >> @@ -1316,11 +1316,32 @@ boolean (e.g. `iommu=no`) can override t
> >>      will prevent Xen from booting if IOMMUs aren't discovered and enabled
> >>      successfully.
> >>
> >> -*   The `quarantine` boolean can be used to control Xen's behavior when
> >> -    de-assigning devices from guests.  If enabled (the default), Xen 
> >> always
> >> +*   The `quarantine` option can be used to control Xen's behavior when
> >> +    de-assigning devices from guests.
> >> +
> >> +    When a PCI device is assigned to an untrusted domain, it is possible
> >> +    for that domain to program the device to DMA to an arbitrary address.
> >> +    The IOMMU is used to protect the host from malicious DMA by making
> >> +    sure that the device addresses can only target memory assigned to the
> >> +    guest.  However, when the guest domain is torn down, assigning the
> >> +    device back to the hardware domain would allow any in-flight DMA to
> >> +    potentially target critical host data.  To avoid this, quarantining
> >> +    should be enabled.  Quarantining can be done in two ways: In its basic
> >> +    form, all in-flight DMA will simply be forced to encounter IOMMU
> >> +    faults.  Since there are systems where doing so can cause host lockup,
> >> +    an alternative form is available where writes to memory will be made
> >> +    fault, but reads will be directed to a dummy page.  The implication
> >> +    here is that such reads will go unnoticed, i.e. an admin may not
> >> +    become aware of the underlying problem.
> >> +
> >> +    Therefore, if this option is set to true (the default), Xen always
> >>      quarantines such devices; they must be explicitly assigned back to 
> >> Dom0
> >> -    before they can be used there again.  If disabled, Xen will only
> >> -    quarantine devices the toolstack hass arranged for getting 
> >> quarantined.
> >> +    before they can be used there again.  If set to "scratch-page", still
> >> +    active DMA reads will additionally be directed to a "scratch" page.  
> >> If
> >
> > There's inconsistency of terms here. We should choose either 'dummy page'
> > or 'scratch page' (and my vote goes for the latter).
> 
> Oh, that wasn't intentional. I've replaced all "dummy" now.
> 
> > Also, rather than true or false, shouldn't we have 'off', 'basic', and
> > 'scratch-page'?
> 
> I didn't want to break (or needlessly extend) the present boolean nature
> of the option. Hence I only added "scratch-page". I wouldn't want to add
> "basic" as an alias of "true", but if you think we really need this, then
> I surely could do so. As to "off" vs "false" - both are permitted anyway
> by the parsing functions. And to me (both as a programmer and as someone
> who had been studying maths long ago) something that's boolean goes
> rather with true/false than on/off; I can certainly change that wording
> if you deem that more appropriate / helpful for the target audience.
> 

I think that once the option gained a third value it ceased to be a boolean and 
hence setting to true/false is really only for compatibility, hence I think an 
'off', 'basic', 'scratch-page' enum would now make more sense (even if 
true/false still works underneath the covers).

  Paul

> Jan