Re: [PATCH v8 5/8] remove remaining uses of iommu_legacy_map/unmap

[Julien Grall <julien@xxxxxxx>]

Hi Paul,

I am sorry for jumping very late in the discussion.

On 11/09/2020 09:20, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@xxxxxxxxxx>
>
> The 'legacy' functions do implicit flushing so amend the callers to do the
> appropriate flushing.
>
> Unfortunately, because of the structure of the P2M code, we cannot remove
> the per-CPU 'iommu_dont_flush_iotlb' global and the optimization it
> facilitates. It is now checked directly iommu_iotlb_flush(). This is safe
> because callers of iommu_iotlb_flush() on another CPU will not be affected,
> and hence a flush will be done. Also, 'iommu_dont_flush_iotlb' is now declared
> as bool (rather than bool_t) and setting/clearing it are no longer pointlessly
> gated on is_iommu_enabled() returning true. (Arguably it is also pointless to
> gate the call to iommu_iotlb_flush() on that condition - since it is a no-op
> in that case - but the if clause allows the scope of a stack variable to be
> restricted).
Unfortunately, this change will re-open a potential security hole closed 
by commit 671878779741:
    xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs

    When freeing a p2m entry, all the sub-tree behind it will also be 
freed.
    This may include intermediate page-tables or any l3 entry requiring to
    drop a reference (e.g for foreign pages). As soon as pages are freed,
    they may be re-used by Xen or another domain. Therefore it is necessary
    to flush *all* the TLBs beforehand.

    While CPU TLBs will be flushed before freeing the pages, this is not
    the case for IOMMU TLBs. This can be solved by moving the IOMMU TLBs
    flush earlier in the code.

    This wasn't considered as a security issue as device passthrough on Arm
    is not security supported.

    Signed-off-by: Julien Grall <julien.grall@xxxxxxx>
    Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>
    Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
    Release-acked-by: Juergen Gross <jgross@xxxxxxxx>

One possibility would be to treat iommu_dont_flush_iotlb as x86 only.

Cheers,

--
Julien Grall