[PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Julien Grall]

The current implementation of raw_copy_* helpers may lead to data corruption
and sometimes Xen crash when the guest virtual address is not aligned to
PAGE_SIZE.

When the total length is higher than a page, the length to read is badly
compute with
    min(len, (unsigned)(PAGE_SIZE - offset))

As the offset is only computed one time per function, if the start address was
not aligned to PAGE_SIZE, we can end up in same iteration:
    - to read accross page boundary => xen crash
    - read the previous page => data corruption

This issue can be resolved by computing the offset on every iteration.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This patch is a bug fix for Xen 4.4. Without this patch the data may be
    corrupted between Xen and the guest when the guest virtual address is
    not aligned to PAGE_SIZE. Sometimes it can also crash Xen.

    These functions are used in numerous place in Xen. If it introduce another
    bug we can see quickly with small amount of data.
---
 xen/arch/arm/guestcopy.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index af0af6b..b3b54e9 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -9,12 +9,11 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
                                               unsigned len, int flush_dcache)
 {
     /* XXX needs to handle faults */
-    unsigned offset = (vaddr_t)to & ~PAGE_MASK;
-
     while ( len )
     {
         paddr_t g;
         void *p;
+        unsigned offset = (vaddr_t)to & ~PAGE_MASK;
         unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
 
         if ( gvirt_to_maddr((vaddr_t) to, &g) )
@@ -50,12 +49,12 @@ unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
 unsigned long raw_clear_guest(void *to, unsigned len)
 {
     /* XXX needs to handle faults */
-    unsigned offset = (vaddr_t)to & ~PAGE_MASK;
 
     while ( len )
     {
         paddr_t g;
         void *p;
+        unsigned offset = (vaddr_t)to & ~PAGE_MASK;
         unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
 
         if ( gvirt_to_maddr((vaddr_t) to, &g) )
@@ -76,12 +75,11 @@ unsigned long raw_clear_guest(void *to, unsigned len)
 
 unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
 {
-    unsigned offset = (vaddr_t)from & ~PAGE_MASK;
-
     while ( len )
     {
         paddr_t g;
         void *p;
+        unsigned offset = (vaddr_t)from & ~PAGE_MASK;
         unsigned size = min(len, (unsigned)(PAGE_SIZE - offset));
 
         if ( gvirt_to_maddr((vaddr_t) from & PAGE_MASK, &g) )
-- 
1.7.10.4

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Stefano Stabellini]

On Fri, 14 Feb 2014, Julien Grall wrote:
> The current implementation of raw_copy_* helpers may lead to data corruption
> and sometimes Xen crash when the guest virtual address is not aligned to
> PAGE_SIZE.
> 
> When the total length is higher than a page, the length to read is badly
> compute with
>     min(len, (unsigned)(PAGE_SIZE - offset))
> 
> As the offset is only computed one time per function, if the start address was
> not aligned to PAGE_SIZE, we can end up in same iteration:
>     - to read accross page boundary => xen crash
>     - read the previous page => data corruption
> 
> This issue can be resolved by computing the offset on every iteration.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>     This patch is a bug fix for Xen 4.4. Without this patch the data may be
>     corrupted between Xen and the guest when the guest virtual address is
>     not aligned to PAGE_SIZE. Sometimes it can also crash Xen.
> 
>     These functions are used in numerous place in Xen. If it introduce another
>     bug we can see quickly with small amount of data.
> ---
>  xen/arch/arm/guestcopy.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index af0af6b..b3b54e9 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -9,12 +9,11 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
>                                                unsigned len, int flush_dcache)
>  {
>      /* XXX needs to handle faults */
> -    unsigned offset = (vaddr_t)to & ~PAGE_MASK;
> -
>      while ( len )
>      {
>          paddr_t g;
>          void *p;
> +        unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>  
>          if ( gvirt_to_maddr((vaddr_t) to, &g) )
> @@ -50,12 +49,12 @@ unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>  unsigned long raw_clear_guest(void *to, unsigned len)
>  {
>      /* XXX needs to handle faults */
> -    unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>  
>      while ( len )
>      {
>          paddr_t g;
>          void *p;
> +        unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>  
>          if ( gvirt_to_maddr((vaddr_t) to, &g) )
> @@ -76,12 +75,11 @@ unsigned long raw_clear_guest(void *to, unsigned len)
>  
>  unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
>  {
> -    unsigned offset = (vaddr_t)from & ~PAGE_MASK;
> -
>      while ( len )
>      {
>          paddr_t g;
>          void *p;
> +        unsigned offset = (vaddr_t)from & ~PAGE_MASK;
>          unsigned size = min(len, (unsigned)(PAGE_SIZE - offset));
>  
>          if ( gvirt_to_maddr((vaddr_t) from & PAGE_MASK, &g) )
> -- 
> 1.7.10.4
> 

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Ian Campbell]

On Fri, 2014-02-14 at 17:10 +0000, Julien Grall wrote:
> The current implementation of raw_copy_* helpers may lead to data corruption
> and sometimes Xen crash when the guest virtual address is not aligned to
> PAGE_SIZE.

Isn't a non-aligned address the vast majority of the cases (hypercall
arguments on the guest stack)? How have we managed to get away with this
for so long?

> When the total length is higher than a page, the length to read is badly
> compute with
>     min(len, (unsigned)(PAGE_SIZE - offset))
> 
> As the offset is only computed one time per function,

We set offset = 0 at the end of the first iteration.

Which I think is correct: On the second iteration things should now be
aligned to a page boundary.

Have you observed offset != 0 for the second and subsequent iterations?

>  if the start address was
> not aligned to PAGE_SIZE, we can end up in same iteration:
>     - to read accross page boundary => xen crash
>     - read the previous page => data corruption
> 
> This issue can be resolved by computing the offset on every iteration.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     This patch is a bug fix for Xen 4.4. Without this patch the data may be
>     corrupted between Xen and the guest when the guest virtual address is
>     not aligned to PAGE_SIZE. Sometimes it can also crash Xen.
> 
>     These functions are used in numerous place in Xen. If it introduce another
>     bug we can see quickly with small amount of data.
> ---
>  xen/arch/arm/guestcopy.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index af0af6b..b3b54e9 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -9,12 +9,11 @@ static unsigned long raw_copy_to_guest_helper(void *to, const void *from,
>                                                unsigned len, int flush_dcache)
>  {
>      /* XXX needs to handle faults */
> -    unsigned offset = (vaddr_t)to & ~PAGE_MASK;
> -
>      while ( len )
>      {
>          paddr_t g;
>          void *p;
> +        unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>  
>          if ( gvirt_to_maddr((vaddr_t) to, &g) )
> @@ -50,12 +49,12 @@ unsigned long raw_copy_to_guest_flush_dcache(void *to, const void *from,
>  unsigned long raw_clear_guest(void *to, unsigned len)
>  {
>      /* XXX needs to handle faults */
> -    unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>  
>      while ( len )
>      {
>          paddr_t g;
>          void *p;
> +        unsigned offset = (vaddr_t)to & ~PAGE_MASK;
>          unsigned size = min(len, (unsigned)PAGE_SIZE - offset);
>  
>          if ( gvirt_to_maddr((vaddr_t) to, &g) )
> @@ -76,12 +75,11 @@ unsigned long raw_clear_guest(void *to, unsigned len)
>  
>  unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned len)
>  {
> -    unsigned offset = (vaddr_t)from & ~PAGE_MASK;
> -
>      while ( len )
>      {
>          paddr_t g;
>          void *p;
> +        unsigned offset = (vaddr_t)from & ~PAGE_MASK;
>          unsigned size = min(len, (unsigned)(PAGE_SIZE - offset));
>  
>          if ( gvirt_to_maddr((vaddr_t) from & PAGE_MASK, &g) )

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Ian Campbell]

On Tue, 2014-02-18 at 14:59 +0000, Ian Campbell wrote:

> > As the offset is only computed one time per function,
> 
> We set offset = 0 at the end of the first iteration.

Ah, we do in raw_copy_to_guest_helper and raw_clear_guest but not
raw_copy_from_guest -- which I think is the actual bug here.

Ian.

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Julien Grall]

On 02/18/2014 02:59 PM, Ian Campbell wrote:
> On Fri, 2014-02-14 at 17:10 +0000, Julien Grall wrote:
>> The current implementation of raw_copy_* helpers may lead to data corruption
>> and sometimes Xen crash when the guest virtual address is not aligned to
>> PAGE_SIZE.
> 
> Isn't a non-aligned address the vast majority of the cases (hypercall
> arguments on the guest stack)? How have we managed to get away with this
> for so long?

Because most of the time the size is smaller than 1 page. It not the
case with flask policy.

-- 
Julien Grall

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Julien Grall]

On 02/18/2014 03:00 PM, Ian Campbell wrote:
> On Tue, 2014-02-18 at 14:59 +0000, Ian Campbell wrote:
> 
>>> As the offset is only computed one time per function,
>>
>> We set offset = 0 at the end of the first iteration.
> 
> Ah, we do in raw_copy_to_guest_helper and raw_clear_guest but not
> raw_copy_from_guest -- which I think is the actual bug here.

I didn't notice the offset = 0 at then end of raw_copy_to_guest.

I can send a patch to only set offset to 0 in raw_copy_from_guest. But I
think it's less clear than this patch. What do you think?

Regards,

-- 
Julien Grall

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Ian Campbell]

On Tue, 2014-02-18 at 15:08 +0000, Julien Grall wrote:
> On 02/18/2014 03:00 PM, Ian Campbell wrote:
> > On Tue, 2014-02-18 at 14:59 +0000, Ian Campbell wrote:
> > 
> >>> As the offset is only computed one time per function,
> >>
> >> We set offset = 0 at the end of the first iteration.
> > 
> > Ah, we do in raw_copy_to_guest_helper and raw_clear_guest but not
> > raw_copy_from_guest -- which I think is the actual bug here.
> 
> I didn't notice the offset = 0 at then end of raw_copy_to_guest.
> 
> I can send a patch to only set offset to 0 in raw_copy_from_guest. But I
> think it's less clear than this patch. What do you think?

I think the approach currently used by the (non-buggy) functions is
better -- it makes it obvious that after the first iteration things
*have* to now be aligned.

I also wouldn't be surprised if the compiler had trouble proving this
and so ended up needlessly recalculating offset instead of optimising it
out.

If you find the code unclear please feel free to add comments etc.

Ian.

Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_*

[Julien Grall]

On 02/18/2014 03:25 PM, Ian Campbell wrote:
> On Tue, 2014-02-18 at 15:08 +0000, Julien Grall wrote:
>> On 02/18/2014 03:00 PM, Ian Campbell wrote:
>>> On Tue, 2014-02-18 at 14:59 +0000, Ian Campbell wrote:
>>>
>>>>> As the offset is only computed one time per function,
>>>>
>>>> We set offset = 0 at the end of the first iteration.
>>>
>>> Ah, we do in raw_copy_to_guest_helper and raw_clear_guest but not
>>> raw_copy_from_guest -- which I think is the actual bug here.
>>
>> I didn't notice the offset = 0 at then end of raw_copy_to_guest.
>>
>> I can send a patch to only set offset to 0 in raw_copy_from_guest. But I
>> think it's less clear than this patch. What do you think?
> 
> I think the approach currently used by the (non-buggy) functions is
> better -- it makes it obvious that after the first iteration things
> *have* to now be aligned.

Ok. I will resend the patch.

> I also wouldn't be surprised if the compiler had trouble proving this
> and so ended up needlessly recalculating offset instead of optimising it
> out.
> 
> If you find the code unclear please feel free to add comments etc.

I will add comment.

-- 
Julien Grall