Re: [Xen-devel] [PATCH v3] IOMMU/x86: make page type checks consistent when mapping pages

[Jan Beulich <JBeulich@xxxxxxxx>]

On 04.07.2019 15:10, Andrew Cooper wrote:
> On 03/07/2019 13:18, Jan Beulich wrote:
>> There are currently three more or less different checks:
>> - _get_page_type() adjusts the IOMMU mappings according to the new type
>>     alone,
>> - arch_iommu_populate_page_table() wants just the type to be
>>     PGT_writable_page,
>> - iommu_hwdom_init() additionally permits all other types with a type
>>     refcount of zero.
>> The canonical one is in _get_page_type(), and as of XSA-288
>> arch_iommu_populate_page_table() also has no need anymore to deal with
>> PGT_none pages. In the PV Dom0 case, however, PGT_none pages are still
>> necessary to consider, since in that case pages don't get handed to
>> guest_physmap_add_entry(). Furthermore, the function so far also
>> established r/o mappings, which is not in line with the rules set forth
>> by the XSA-288 change.
>>
>> For arch_iommu_populate_page_table() to not encounter PGT_none pages
>> anymore even in cases where the IOMMU gets enabled for a domain only
>> when it is already running, replace the IOMMU dependency in
>> guest_physmap_add_entry()'s handling of PV guests to check just the
>> system wide state instead of the domain property.
> 
> And this is the problem.  We have an enormous amount of complexity, and
> a hypercall which even after an XSA, we could only reduce to "will
> livelock under adversarial conditions (inc. the toolstack thread which
> started it)", so support a corner case which doesn't (AFACIT) look like
> it can work correctly, and surely isn't used in practice.
> 
> IMO, the only sane thing to do is to have a "create an IOMMU context"
> flag in domaincreate (and a shared vs split while we're at it, for the
> EPT case), and have the IOMMU context properly known from the very start
> of the domain.

Irrespective of me disagreeing here (which by no means suggests that
what you describe may not get done anyway), ...

> Even if this does end up restricting a corner case which is believed to
> work, I do not see it as an inconvenience or problem to require a domain
> config file to specify whether it wants an IOMMU context (directly, or
> indirectly via things like PCI=), and the reduction in complexity in Xen
> would be massive.
> 
> How many security bugs have we already found here?  How may are still
> lurking, or liable to be re-introduced because this code is just too
> damn complicated for you, me and George to review sensibly?  Reducing
> the complexity is the responsible thing to do.
> 
> I'm afraid that I don't view anything other than moving towards an
> up-front declaration as making the situation better.

... I'm pretty puzzled by this, in particular seeing that the bulk of
the change in this patch is affecting Dom0 only, i.e. is in no way
related to the point in time at which we declare a DomU to need IOMMU
page tables. The change affecting DomU-s here is a single line, plus
some comment adjustment.

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