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

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

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@xxxxxxxxxx>
> 
> ---
>     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) )



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel