From: Julien Grall <jgrall@amazon.com>

... 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 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@arm.com>
Fixes: 552710b38863 ("xen/arm: grant: Add another entry to map MFN 1:1 in dom0 p2m")
Signed-off-by: Julien Grall <jgrall@amazon.com>

---

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

On 14.02.2021 15:35, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> ... 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 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.

Doesn't this want / need solving in grant_table.c itself, as it also
affects PV on x86? Or alternatively in gnttab_need_iommu_mapping(),
handing the macro the MFN alongside the domain? No matter which one
was chosen, it could at the same time avoid the expensive mapkind()
invocation in this case.

Jan

On Mon, 15 Feb 2021, Jan Beulich wrote:
> On 14.02.2021 15:35, Julien Grall wrote:
> > From: Julien Grall <jgrall@amazon.com>
> > 
> > ... 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 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.
> 
> Doesn't this want / need solving in grant_table.c itself, as it also
> affects PV on x86? Or alternatively in gnttab_need_iommu_mapping(),
> handing the macro the MFN alongside the domain? No matter which one
> was chosen, it could at the same time avoid the expensive mapkind()
> invocation in this case.

Not knowing the x86 side I don't have an opinion on the best location
for the check. But I wanted to say for the records that the patch has
already been tested successfully and looks good to me.

Hello Julien,

> On 14 Feb 2021, at 2:35 pm, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> ... 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@arm.com>
> Fixes: 552710b38863 ("xen/arm: grant: Add another entry to map MFN 1:1 in dom0 p2m")
> Signed-off-by: Julien Grall <jgrall@amazon.com>

With the typo fixed.

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

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
>