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

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 12/05/2021 23:00, Stefano Stabellini wrote:
> 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
>
> ?
I thought about it but I didn't want to define to a random value... I 
felt not define it was better.
> > 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 )
This would not protect against an overflow. So I think it is best if we 
keep my version.
Cheers,

--
Julien Grall