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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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.

~Andrew

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