[PATCH v2] xen/arm: Correctly handle non-page aligned pointer in raw_copy_from_guest

[PATCH v2] xen/arm: Correctly handle non-page aligned pointer in raw_copy_from_guest

[Julien Grall]

The current implementation of raw_copy_guest helper 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 setting offset to 0 at the end of the first
iteration. Indeed, after it, the virtual guest address is always aligned
to PAGE_SIZE.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Cc: George Dunlap <george.dunlap@citrix.com>

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

    This function is used in numerous place in Xen. If it introduces another
    bug we can see quickly with small amount of data.

    Changes in v2:
        - Only raw_copy_from_guest is buggy, the other raw_copy_*
        helpers where safe because of the "offset = 0" at the end of the loop
        - Update commit message and title
---
 xen/arch/arm/guestcopy.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
index af0af6b..715bb4e 100644
--- a/xen/arch/arm/guestcopy.c
+++ b/xen/arch/arm/guestcopy.c
@@ -96,6 +96,11 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned le
         len -= size;
         from += size;
         to += size;
+        /*
+         * After the first iteration, guest virtual address is correctly
+         * aligned to PAGE_SIZE.
+         */
+        offset = 0;
     }
     return 0;
 }
-- 
1.7.10.4

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

[Ian Campbell]

On Tue, 2014-02-18 at 16:56 +0000, Julien Grall wrote:
> The current implementation of raw_copy_guest helper 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 setting offset to 0 at the end of the first
> iteration. Indeed, after it, the virtual guest address is always aligned
> to PAGE_SIZE.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>

Acked-by: Ian Campbell <ian.campbell@citrix.com>


> Cc: George Dunlap <george.dunlap@citrix.com>
> 
> ---
>     This patch is a bug fix for Xen 4.4. Without this patch the data may be
>     corrupted when Xen is copied data from the guest if the guest virtual
>     address is not aligned to PAGE_SIZE. Sometimes it can also crash Xen.

The crash is what makes this undoubtedly necessary for 4.4 IMHO.

>     This function is used in numerous place in Xen. If it introduces another
>     bug we can see quickly with small amount of data.
> 
>     Changes in v2:
>         - Only raw_copy_from_guest is buggy, the other raw_copy_*
>         helpers where safe because of the "offset = 0" at the end of the loop
>         - Update commit message and title
> ---
>  xen/arch/arm/guestcopy.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index af0af6b..715bb4e 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -96,6 +96,11 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned le
>          len -= size;
>          from += size;
>          to += size;
> +        /*
> +         * After the first iteration, guest virtual address is correctly
> +         * aligned to PAGE_SIZE.
> +         */

I'd like to duplicate this comment in the other two places too -- if you
are OK with it I will do that as part of committing.

> +        offset = 0;
>      }
>      return 0;
>  }

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

[Ian Campbell]

On Tue, 2014-02-18 at 17:10 +0000, Ian Campbell wrote:
> On Tue, 2014-02-18 at 16:56 +0000, Julien Grall wrote:
> > The current implementation of raw_copy_guest helper 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 setting offset to 0 at the end of the first
> > iteration. Indeed, after it, the virtual guest address is always aligned
> > to PAGE_SIZE.
> > 
> > Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

and applied.

> > +        /*
> > +         * After the first iteration, guest virtual address is correctly
> > +         * aligned to PAGE_SIZE.
> > +         */
> 
> I'd like to duplicate this comment in the other two places too -- if you
> are OK with it I will do that as part of committing.

I did this.

Ian.

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

[Julien Grall]

On 02/18/2014 05:46 PM, Ian Campbell wrote:
> On Tue, 2014-02-18 at 17:10 +0000, Ian Campbell wrote:
>> On Tue, 2014-02-18 at 16:56 +0000, Julien Grall wrote:
>>> The current implementation of raw_copy_guest helper 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 setting offset to 0 at the end of the first
>>> iteration. Indeed, after it, the virtual guest address is always aligned
>>> to PAGE_SIZE.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> and applied.
> 
>>> +        /*
>>> +         * After the first iteration, guest virtual address is correctly
>>> +         * aligned to PAGE_SIZE.
>>> +         */
>>
>> I'd like to duplicate this comment in the other two places too -- if you
>> are OK with it I will do that as part of committing.
> 
> I did this.

Thanks! I didn't see this part on the previous message.

-- 
Julien Grall

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

[George Dunlap]

On 02/18/2014 04:56 PM, Julien Grall wrote:
> The current implementation of raw_copy_guest helper 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 setting offset to 0 at the end of the first
> iteration. Indeed, after it, the virtual guest address is always aligned
> to PAGE_SIZE.
>
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Cc: George Dunlap <george.dunlap@citrix.com>
>
> ---
>      This patch is a bug fix for Xen 4.4. Without this patch the data may be
>      corrupted when Xen is copied data from the guest if the guest virtual
>      address is not aligned to PAGE_SIZE. Sometimes it can also crash Xen.
>
>      This function is used in numerous place in Xen. If it introduces another
>      bug we can see quickly with small amount of data.

Release-acked-by: George Dunlap <george.dunlap@eu.citrix.com>

>
>      Changes in v2:
>          - Only raw_copy_from_guest is buggy, the other raw_copy_*
>          helpers where safe because of the "offset = 0" at the end of the loop
>          - Update commit message and title
> ---
>   xen/arch/arm/guestcopy.c |    5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/xen/arch/arm/guestcopy.c b/xen/arch/arm/guestcopy.c
> index af0af6b..715bb4e 100644
> --- a/xen/arch/arm/guestcopy.c
> +++ b/xen/arch/arm/guestcopy.c
> @@ -96,6 +96,11 @@ unsigned long raw_copy_from_guest(void *to, const void __user *from, unsigned le
>           len -= size;
>           from += size;
>           to += size;
> +        /*
> +         * After the first iteration, guest virtual address is correctly
> +         * aligned to PAGE_SIZE.
> +         */
> +        offset = 0;
>       }
>       return 0;
>   }