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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

+ Juergen

On Mon, 12 Aug 2019, 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@xxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>


> ---
> 
> Cc: olekstysh@xxxxxxxxx
> Cc: oleksandr_tyshchenko@xxxxxxxx
> 
>     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.
> 
>     One of the solution would be to:
>        1) Have a pool of memory for each domain p2m page-tables. So the
>        domain can only touch itself
>        2) Defer foreign mapping removal
> 
>     1) should also solve the case where the P2M is trying to shatter
>     everything and therefore hog the memory. Note that today we don't
>     free empty page-tables.
> 
>     2) I always felt trying to remove the foreign page reference in the
>     p2m code was wrong. This is done because we currently allow the
>     guest to remove any mapping. So we need to protect ourself against a
>     rogue guest. We could try to restrict what the guest can do on the
>     p2m.
> ---
>  xen/arch/arm/p2m.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
> index 3c8287a048..963cd1d600 100644
> --- a/xen/arch/arm/p2m.c
> +++ b/xen/arch/arm/p2m.c
> @@ -1048,14 +1048,6 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>          p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
>      }
>  
> -    /*
> -     * Free the entry only if the original pte was valid and the base
> -     * is different (to avoid freeing when permission is changed).
> -     */
> -    if ( p2m_is_valid(orig_pte) &&
> -         !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
> -        p2m_free_entry(p2m, orig_pte, level);
> -
>      if ( has_iommu_pt(p2m->domain) &&
>           (lpae_is_valid(orig_pte) || lpae_is_valid(*entry)) )
>      {
> @@ -1072,6 +1064,14 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
>      else
>          rc = 0;
>  
> +    /*
> +     * Free the entry only if the original pte was valid and the base
> +     * is different (to avoid freeing when permission is changed).
> +     */
> +    if ( p2m_is_valid(orig_pte) &&
> +         !mfn_eq(lpae_get_mfn(*entry), lpae_get_mfn(orig_pte)) )
> +        p2m_free_entry(p2m, orig_pte, level);
> +
>  out:
>      unmap_domain_page(table);
>  
> -- 
> 2.11.0
> 

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