diff mbox series

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

Message ID 20210425201318.15447-9-julien@xen.org (mailing list archive)
State New
Headers show
Series xen/arm: mm: Remove open-coding mappings | expand

Commit Message

Julien Grall April 25, 2021, 8:13 p.m. UTC
From: Julien Grall <jgrall@amazon.com>

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
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
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@amazon.com>

---
    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(-)

Comments

Stefano Stabellini May 12, 2021, 10 p.m. UTC | #1
On Sun, 25 Apr 2021, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> 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@amazon.com>
> 
> ---
>     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
>
Julien Grall May 12, 2021, 10:23 p.m. UTC | #2
Hi Stefano,

On 12/05/2021 23:00, Stefano Stabellini wrote:
> On Sun, 25 Apr 2021, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> 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@amazon.com>
>>
>> ---
>>      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,
Stefano Stabellini May 13, 2021, 10:32 p.m. UTC | #3
On Wed, 12 May 2021, Julien Grall wrote:
> Hi Stefano,
> 
> On 12/05/2021 23:00, Stefano Stabellini wrote:
> > On Sun, 25 Apr 2021, Julien Grall wrote:
> > > From: Julien Grall <jgrall@amazon.com>
> > > 
> > > 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.

Yeah, I see your point: any restrictions in addressing (e.g. 48bits)
are physical address restrictions. Here we are talking about virtual
address restriction, and I don't think there are actually any
restrictions there?  We could validly map something at
0xffff_ffff_ffff_ffff. So even (1<<48) which makes sense at the physical
level, doesn't make sense in terms of virtual addresses.


> > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > > 
> > > ---
> > >      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.

But there can be no overflow with the way SHARED_VIRT_END is defined.
Even if SHARED_VIRT_END was defined at (1<<48) there can be no overflow.
Only if we defined SHARED_VIRT_END as 0xffff_ffff_ffff_ffff we would
have an overflow, but you wrote above that your preference is not to do
that.
Julien Grall May 13, 2021, 10:59 p.m. UTC | #4
On Thu, 13 May 2021, 23:32 Stefano Stabellini, <sstabellini@kernel.org>
wrote:

> On Wed, 12 May 2021, Julien Grall wrote:
> > Hi Stefano,
> >
> > On 12/05/2021 23:00, Stefano Stabellini wrote:
> > > On Sun, 25 Apr 2021, Julien Grall wrote:
> > > > From: Julien Grall <jgrall@amazon.com>
> > > >
> > > > 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.
>
> Yeah, I see your point: any restrictions in addressing (e.g. 48bits)
> are physical address restrictions. Here we are talking about virtual
> address restriction, and I don't think there are actually any
> restrictions there?  We could validly map something at
> 0xffff_ffff_ffff_ffff. So even (1<<48) which makes sense at the physical
> level, doesn't make sense in terms of virtual addresses.
>

The limit for the virtual address is 2^64.


>
> > > > Signed-off-by: Julien Grall <jgrall@amazon.com>
> > > >
> > > > ---
> > > >      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.
>
> But there can be no overflow with the way SHARED_VIRT_END is defined.

Even if SHARED_VIRT_END was defined at (1<<48) there can be no overflow.
> Only if we defined SHARED_VIRT_END as 0xffff_ffff_ffff_ffff we would
> have an overflow, but you wrote above that your preference is not to do
> that.
>

You can have an overflow regardless the value of SHARED_VIRT_END.

Imagine virt = 2^64 - 1 and nr_mfs = 1. The addition would result to 0.

As a consequence the check would pass when it should not.

One can argue that the caller will always provide sane values. However
given the simplicity of the check, this is not worth the trouble if a
caller is buggy.

Now, the problem with SHARED_VIRT_END equals to 2^64 - 1 is not the
overflow but the compiler that may throw an error/warning for always true
check. Hence the reason of not defining SHARED_VIRT_END on arm64.

Cheers,
Stefano Stabellini May 14, 2021, 1:04 a.m. UTC | #5
On Thu, 13 May 2021, Julien Grall wrote:
> On Thu, 13 May 2021, 23:32 Stefano Stabellini, <sstabellini@kernel.org> wrote:
>       On Wed, 12 May 2021, Julien Grall wrote:
>       > Hi Stefano,
>       >
>       > On 12/05/2021 23:00, Stefano Stabellini wrote:
>       > > On Sun, 25 Apr 2021, Julien Grall wrote:
>       > > > From: Julien Grall <jgrall@amazon.com>
>       > > >
>       > > > 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.
> 
>       Yeah, I see your point: any restrictions in addressing (e.g. 48bits)
>       are physical address restrictions. Here we are talking about virtual
>       address restriction, and I don't think there are actually any
>       restrictions there?  We could validly map something at
>       0xffff_ffff_ffff_ffff. So even (1<<48) which makes sense at the physical
>       level, doesn't make sense in terms of virtual addresses.
> 
> 
> The limit for the virtual address is 2^64.
> 
> 
> 
>       > > > Signed-off-by: Julien Grall <jgrall@amazon.com>
>       > > >
>       > > > ---
>       > > >      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.
> 
>       But there can be no overflow with the way SHARED_VIRT_END is defined.
> 
>       Even if SHARED_VIRT_END was defined at (1<<48) there can be no overflow.
>       Only if we defined SHARED_VIRT_END as 0xffff_ffff_ffff_ffff we would
>       have an overflow, but you wrote above that your preference is not to do
>       that.
> 
> 
> You can have an overflow regardless the value of SHARED_VIRT_END.
> 
> Imagine virt = 2^64 - 1 and nr_mfs = 1. The addition would result to 0.
> 
> As a consequence the check would pass when it should not.

Yes you are right, I don't know how I missed it!


> One can argue that the caller will always provide sane values. However given the simplicity of the check, this is not worth the trouble if
> a caller is buggy.
> 
> Now, the problem with SHARED_VIRT_END equals to 2^64 - 1 is not the overflow but the compiler that may throw an error/warning for always
> true check. Hence the reason of not defining SHARED_VIRT_END on arm64.

OK, all checks out.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
diff mbox series

Patch

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 )
+    {
+        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)