Re: [Xen-devel] [PATCH] xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Julien Grall <julien.grall@arm.com>, <xen-devel@lists.xenproject.org>
Cc: olekstysh@gmail.com, oleksandr_tyshchenko@epam.com,
	Stefano Stabellini <sstabellini@kernel.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [Xen-devel] [PATCH] xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs
Date: Tue, 13 Aug 2019 09:03:07 +0100	[thread overview]
Message-ID: <d904c960-3a9b-bcfc-1631-5be88f793862@citrix.com> (raw)
In-Reply-To: <20190812202735.23411-1-julien.grall@arm.com>

On 12/08/2019 21:27, Julien Grall wrote:
> 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@arm.com>
>
> ---
>
> Cc: olekstysh@gmail.com
> Cc: oleksandr_tyshchenko@epam.com
>
>     I discovered it while looking at the code, so I don't have any
>     reproducer of the issue. There is a small windows where page could
>     be reallocated to Xen or another domain but still present in the
>     IOMMU TLBs.
>
>     This patch only address the case where the flush succeed. In the
>     unlikely case where it does not succeed, then we will still free the
>     pages. The IOMMU helper will crash domain, but the device may still
>     not be quiescent. So there are a potentially issues do DMA on wrong
>     things.
>
>     At the moment, none of the Arm IOMMUs drivers (including the IPMMU
>     one under review) are return an error here. Note that flush may
>     still fail (see timeout), but is ignored. This is not great as it
>     means a device may DMA into something that does not belong to the
>     domain. So we probably want to return an error here.
>
>     Even if an error is returned, there are still potential issues
>     (see above). The fix is not entirely trivial, we would need to keep
>     the page around until the a device is quiescent or the IOMMU is
>     reset. This mostly likely means until the domain is fully destroyed.

Xen's behaviour with IOMMU timeouts is broken, and definitely unsafe.

We do not (and indeed must not) impose a timeout for TLB flush
operations locally in the core.  IOTLB flush operations are no different.

If the IOMMU starts malfunctioning, that is fatal to the whole system,
not just the guest in question.

The only viable approach is to drop the artificial timeouts and up the
severity of a malfunction.

~Andrew

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