Re: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared before updating it
Hi, On 21/02/2022 10:22, Julien Grall wrote: From: Julien Grall Only the first 2GB of the virtual address space is shared between all the page-tables on Arm32. There is a long outstanding TODO in xen_pt_update() stating that the function can only work with shared mapping. Nobody has ever called the function with private mapping, however as we add more callers there is a risk to mess things up. Introduce a new define to mark the end of the shared mappings and use it in xen_pt_update() to verify if the address is correct. Note that on Arm64, all the mappings are shared. Some compiler may complain about an always true check, so the new define is not introduced for arm64 and the code is protected with an #ifdef. Signed-off-by: Julien Grall --- Changes in v2: - New patch --- xen/arch/arm/include/asm/config.h | 4 xen/arch/arm/mm.c | 11 +-- While I working on removing the identity mapping, I realized this patch is actually getting in my way for arm32. I am planning to have multiple region that are shared, but still a single unshared region (the domheap mapping area). So I will rework the patch to check is the address is part of the unshared region. I will drop the patch from the series and move it to the next one. Cheers, -- Julien Grall
Re: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared before updating it
On 18/03/2022 10:44, Hongda Deng wrote: Hi Julien, Hi Hongda, -Original Message- From: Xen-devel On Behalf Of Julien Grall Sent: 2022年2月21日 18:22 To: xen-devel@lists.xenproject.org Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini ; Bertrand Marquis ; Volodymyr Babchuk Subject: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared before updating it From: Julien Grall Only the first 2GB of the virtual address space is shared between all the page-tables on Arm32. There is a long outstanding TODO in xen_pt_update() stating that the function can only work with shared mapping. Nobody has ever called the function with private mapping, however as we add more callers there is a risk to mess things up. Introduce a new define to mark the end of the shared mappings and use it in xen_pt_update() to verify if the address is correct. Note that on Arm64, all the mappings are shared. Some compiler may complain about an always true check, so the new define is not introduced for arm64 and the code is protected with an #ifdef. Signed-off-by: Julien Grall --- Changes in v2: - New patch --- xen/arch/arm/include/asm/config.h | 4 xen/arch/arm/mm.c | 11 +-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/xen/arch/arm/include/asm/config.h b/xen/arch/arm/include/asm/config.h index c7b77912013e..85d4a510ce8a 100644 --- a/xen/arch/arm/include/asm/config.h +++ b/xen/arch/arm/include/asm/config.h @@ -137,6 +137,10 @@ #define XENHEAP_VIRT_START _AT(vaddr_t,0x4000) #define XENHEAP_VIRT_END _AT(vaddr_t,0x7fff) + +/* The first 2GB is always shared between all the page-tables. */ +#define SHARED_VIRT_END_AT(vaddr_t, 0x7fff) + #define DOMHEAP_VIRT_START _AT(vaddr_t,0x8000) #define DOMHEAP_VIRT_END _AT(vaddr_t,0x) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 24de8dcb9042..f18f65745595 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -1365,11 +1365,18 @@ static int xen_pt_update(unsigned long virt, * For arm32, page-tables are different on each CPUs. Yet, they share * some common mappings. It is assumed that only common mappings * will be modified with this function. - * - * XXX: Add a check. */ const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE); +#ifdef SHARED_VIRT_END +if ( virt > SHARED_VIRT_END || + (SHARED_VIRT_END - virt) < nr_mfns ) Why not convert (SHARED_VIRT_END - virt) to page number before comparation? I think nr_mfns is something related to page numbers, so maybe something like PAGE_SHIFT or round_pgdown is needed. You are correct. nr_mfns should be shifted by PAGE_SHIFT. I have updated check to: (SHARED_VIRT_END - virt) < pfn_to_paddr(nr_mfns) Thanks for spotting it! Cheers, -- Julien Grall
RE: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared before updating it
Hi Julien,
> -Original Message-
> From: Xen-devel On Behalf Of Julien
> Grall
> Sent: 2022年2月21日 18:22
> To: xen-devel@lists.xenproject.org
> Cc: jul...@xen.org; Julien Grall ; Stefano Stabellini
> ; Bertrand Marquis ;
> Volodymyr Babchuk
> Subject: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is
> shared
> before updating it
>
> From: Julien Grall
>
> Only the first 2GB of the virtual address space is shared between all
> the page-tables on Arm32.
>
> There is a long outstanding TODO in xen_pt_update() stating that the
> function can only work with shared mapping. Nobody has ever called
> the function with private mapping, however as we add more callers
> there is a risk to mess things up.
>
> Introduce a new define to mark the end of the shared mappings and use
> it in xen_pt_update() to verify if the address is correct.
>
> Note that on Arm64, all the mappings are shared. Some compiler may
> complain about an always true check, so the new define is not introduced
> for arm64 and the code is protected with an #ifdef.
>
> Signed-off-by: Julien Grall
>
> ---
> Changes in v2:
> - New patch
> ---
> xen/arch/arm/include/asm/config.h | 4
> xen/arch/arm/mm.c | 11 +--
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/include/asm/config.h
> b/xen/arch/arm/include/asm/config.h
> index c7b77912013e..85d4a510ce8a 100644
> --- a/xen/arch/arm/include/asm/config.h
> +++ b/xen/arch/arm/include/asm/config.h
> @@ -137,6 +137,10 @@
>
> #define XENHEAP_VIRT_START _AT(vaddr_t,0x4000)
> #define XENHEAP_VIRT_END _AT(vaddr_t,0x7fff)
> +
> +/* The first 2GB is always shared between all the page-tables. */
> +#define SHARED_VIRT_END_AT(vaddr_t, 0x7fff)
> +
> #define DOMHEAP_VIRT_START _AT(vaddr_t,0x8000)
> #define DOMHEAP_VIRT_END _AT(vaddr_t,0x)
>
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 24de8dcb9042..f18f65745595 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1365,11 +1365,18 @@ static int xen_pt_update(unsigned long virt,
> * For arm32, page-tables are different on each CPUs. Yet, they share
> * some common mappings. It is assumed that only common mappings
> * will be modified with this function.
> - *
> - * XXX: Add a check.
> */
> const mfn_t root = virt_to_mfn(THIS_CPU_PGTABLE);
>
> +#ifdef SHARED_VIRT_END
> +if ( virt > SHARED_VIRT_END ||
> + (SHARED_VIRT_END - virt) < nr_mfns )
Why not convert (SHARED_VIRT_END - virt) to page number before comparation?
I think nr_mfns is something related to page numbers, so maybe something like
PAGE_SHIFT or round_pgdown is needed.
I am just wondering, and forgive me if I am wrong.
> +{
> +mm_printk("Trying to map outside of the shared area.\n");
> +return -EINVAL;
> +}
> +#endif
> +
> /*
> * The hardware was configured to forbid mapping both writeable and
> * executable.
> --
> 2.32.0
>
Cheers,
---
Hongda
