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

[Stefano Stabellini <stefano.stabellini@eu.citrix.com>]

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
>