Re: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared before updating it

[Julien Grall <julien@xxxxxxx>]

On 18/03/2022 10:44, Hongda Deng wrote:
> Hi Julien,
Hi Hongda,

> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Julien
> > Grall
> > Sent: 2022年2月21日 18:22
> > To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> > Cc: julien@xxxxxxx; Julien Grall <jgrall@xxxxxxxxxx>; Stefano Stabellini
> > <sstabellini@xxxxxxxxxx>; Bertrand Marquis <Bertrand.Marquis@xxxxxxx>;
> > Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> > Subject: [PATCH v3 09/19] xen/arm32: mm: Check if the virtual address is shared
> > before updating it
> >
> > 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 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 <jgrall@xxxxxxxxxx>
> >
> > ---
> >      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,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)
> >
> > 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