Re: [PATCH RFCv2 08/15] xen/arm32: mm: Check if the virtual address is shared before updating it

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Sun, 25 Apr 2021, Julien Grall wrote:
> From: Julien Grall <jgrall@xxxxxxxxxx>
> 
> 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 is can only work with shared mapping. Nobody has ever called
           ^ remove

> 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 ened of the shared mappings and use
                                     ^end

> 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.
 
On arm64 we could maybe define SHARED_VIRT_END to an arbitrarely large
value, such as:

#define SHARED_VIRT_END (1UL<<48)

or:

#define SHARED_VIRT_END DIRECTMAP_VIRT_END

?


> Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> ---
>     Changes in v2:
>         - New patch
> ---
>  xen/arch/arm/mm.c            | 11 +++++++++--
>  xen/include/asm-arm/config.h |  4 ++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 8fac24d80086..5c17cafff847 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -1275,11 +1275,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 )

The following would be sufficient, right?

    if ( virt + nr_mfns > SHARED_VIRT_END )


> +    {
> +        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.
> diff --git a/xen/include/asm-arm/config.h b/xen/include/asm-arm/config.h
> index c7b77912013e..85d4a510ce8a 100644
> --- a/xen/include/asm-arm/config.h
> +++ b/xen/include/asm-arm/config.h
> @@ -137,6 +137,10 @@
>  
>  #define XENHEAP_VIRT_START     _AT(vaddr_t,0x40000000)
>  #define XENHEAP_VIRT_END       _AT(vaddr_t,0x7fffffff)
> +
> +/* The first 2GB is always shared between all the page-tables. */
> +#define SHARED_VIRT_END        _AT(vaddr_t, 0x7fffffff)
> +
>  #define DOMHEAP_VIRT_START     _AT(vaddr_t,0x80000000)
>  #define DOMHEAP_VIRT_END       _AT(vaddr_t,0xffffffff)
>  
> -- 
> 2.17.1
>