Re: [PATCH] xen/iommu: arm: Don't insert an IOMMU mapping when the grantee and granter...

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hello Julien,

> On 14 Feb 2021, at 2:35 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ... are the same.
> 
> When the IOMMU is enabled and the domain is direct mapped (e.g. Dom0),
> Xen will insert a 1:1 mapping for each grant mapping in the P2M to
> allow DMA.
> 
> This works quite well when the grantee and granter and

/s/and/are

> not the same
> because the GFN in the P2M should not be mapped. However, if they are
> the same, we will overwrite the mapping. Worse, it will be completely
> removed when the grant is unmapped.
> 
> As the domain is direct mapped, a 1:1 mapping should always present in
> the P2M. This is not 100% guaranteed if the domain decides to mess with
> the P2M. However, such domain would already end up in trouble as the
> page would be soon be freed (when the last reference dropped).
> 
> Add an additional check in arm_iommu_{,un}map_page() to check whether
> the page belongs to the domain. If it is belongs to it, then ignore the
> request.
> 
> Note that we don't need to hold an extra reference on the page because
> the grant code will already do it for us.
> 
> Reported-by: Rahul Singh <Rahul.Singh@xxxxxxx>
> Fixes: 552710b38863 ("xen/arm: grant: Add another entry to map MFN 1:1 in 
> dom0 p2m")
> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>

With the typo fixed.

Reviewed-by: Rahul Singh <rahul.singh@xxxxxxx>
Tested-by: Rahul Singh <rahul.singh@xxxxxxx>

Regards,
Rahul
> 
> ---
> 
> This is a candidate for 4.15. Without the patch, it would not be
> possible to get the frontend and backend in the same domain (useful when
> trying to map the guest block device in dom0).
> ---
> xen/drivers/passthrough/arm/iommu_helpers.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
> 
> diff --git a/xen/drivers/passthrough/arm/iommu_helpers.c 
> b/xen/drivers/passthrough/arm/iommu_helpers.c
> index a36e2b8e6c42..35a813308b8c 100644
> --- a/xen/drivers/passthrough/arm/iommu_helpers.c
> +++ b/xen/drivers/passthrough/arm/iommu_helpers.c
> @@ -53,6 +53,17 @@ int __must_check arm_iommu_map_page(struct domain *d, 
> dfn_t dfn, mfn_t mfn,
> 
>     t = (flags & IOMMUF_writable) ? p2m_iommu_map_rw : p2m_iommu_map_ro;
> 
> +    /*
> +     * The granter and grantee can be the same domain. In normal
> +     * condition, the 1:1 mapping should already present in the P2M so
> +     * we want to avoid overwriting it.
> +     *
> +     * Note that there is no guarantee the 1:1 mapping will be present
> +     * if the domain decides to mess with the P2M.
> +     */
> +    if ( page_get_owner(mfn_to_page(mfn)) == d )
> +        return 0;
> +
>     /*
>      * The function guest_physmap_add_entry replaces the current mapping
>      * if there is already one...
> @@ -71,6 +82,13 @@ int __must_check arm_iommu_unmap_page(struct domain *d, 
> dfn_t dfn,
>     if ( !is_domain_direct_mapped(d) )
>         return -EINVAL;
> 
> +    /*
> +     * When the granter and grantee are the same, we didn't insert a
> +     * mapping. So ignore the unmap request.
> +     */
> +    if ( page_get_owner(mfn_to_page(_mfn(dfn_x(dfn)))) == d )
> +        return 0;
> +
>     return guest_physmap_remove_page(d, _gfn(dfn_x(dfn)), _mfn(dfn_x(dfn)), 
> 0);
> }
> 
> -- 
> 2.17.1
>