[Xen-devel] 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@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.

    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(-)

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

Hi Andrew,

On 8/13/19 9:03 AM, Andrew Cooper wrote:
> 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.

Well there is a difference between CPU TLB flush and IOTLB flush. For 
the former, the dsb isb sequence is enough to ensure the completion.

For the latter you need to poll a register to check the completion.

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

Whether this is a fatal or not, we need to detect it and make sure it 
does not do more damage on the systems.

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

I am not sure to understand your suggestion... If you drop the timeout, 
how do you detect an IOMMU starts malfunctioning?

Cheers,

On 12.08.19 23:27, Julien Grall wrote:

Hi, Julien

> 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.

I haven't reproduced this issue as well.

So, my testing here is rather formal to be sure that patch doesn't break 
anything.


You can add (if needed):

Tested-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


>      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.

Makes sense.


[I haven't been facing flush timeout issue since start playing with 
IPMMU...]

+ 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@arm.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> 
> 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.
> 
>     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
>

On 02.10.19 04:07, Stefano Stabellini wrote:
> + 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@arm.com>
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen