Re: [Xen-devel] [PATCH v3] IOMMU: make DMA containment of quarantined devices optional

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2020 09:58, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: 09 March 2020 11:09
>>
>> @@ -1276,11 +1276,15 @@ 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
>> -    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.
>> +*   The `quarantine` option can be used to control Xen's behavior when
>> +    de-assigning devices from guests.  If 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 set to "full", still
>> +    active DMA will additionally be directed to a "sink" page.
> 
> I realise this is only in the diff context, but I'm not sure what the 
> following sentence actually means:
> 
>>  If set to
>> +    false, Xen will only quarantine devices the toolstack has arranged for
>> +    getting quarantined.
>> +
> 
> Sounds tautological to me.

Not to me - "false" could also mean no quarantining at all (i.e. no
tool stack control).

>> --- a/xen/drivers/passthrough/Kconfig
>> +++ b/xen/drivers/passthrough/Kconfig
>> @@ -28,3 +28,31 @@ endif
>>
>>  config IOMMU_FORCE_PT_SHARE
>>      bool
>> +
>> +choice
>> +    prompt "IOMMU device quarantining default behavior"
>> +    depends on HAS_PCI
>> +    default IOMMU_QUARANTINE_BASIC
>> +    ---help---
>> +      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
>> +      shold be enabled.
> 
> IMO the above text is a good summary and it would be useful it were
> duplicated in the command line documentation.

Can do.

>>  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.
>> +
>> +    config IOMMU_QUARANTINE_NONE
>> +            bool "none"
>> +    config IOMMU_QUARANTINE_BASIC
>> +            bool "basic"
>> +    config IOMMU_QUARANTINE_FULL
>> +            bool "full"
> 
> 'scratch_page' perhaps? Seems a bit more self-explanatory.

But it still wouldn't carry the "reads only" aspect. I'll reply to
Kevin later, where this aspect will be of more interest. But yes,
I'll think some more about perhaps using that.

>> --- a/xen/drivers/passthrough/iommu.c
>> +++ b/xen/drivers/passthrough/iommu.c
>> @@ -31,9 +31,24 @@ bool_t __initdata iommu_enable = 1;
>>  bool_t __read_mostly iommu_enabled;
>>  bool_t __read_mostly force_iommu;
>>  bool_t __read_mostly iommu_verbose;
>> -bool __read_mostly iommu_quarantine = true;
>>  bool_t __read_mostly iommu_crash_disable;
>>
>> +#define IOMMU_quarantine_none        0 /* aka false */
>> +#define IOMMU_quarantine_fault       1 /* aka true */
>> +#define IOMMU_quarantine_write_fault 2
> 
> enum for the above perhaps?

Well, I don't want the variable to become wider than a byte. And I
couldn't really settle between the ugliness of not using an enum
and the ugliness of attaching __packed to it. If you have a clear
preference for a packed enum, I'll switch - just let me know.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel