From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ian Campbell Subject: Re: [PATCH] xen/arm: Correctly handle non-page aligned pointer in raw_copy_* Date: Tue, 18 Feb 2014 14:59:36 +0000 Message-ID: <1392735576.11080.87.camel@kazak.uk.xensource.com> References: <1392397809-13255-1-git-send-email-julien.grall@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WFm97-0001W4-BQ for xen-devel@lists.xenproject.org; Tue, 18 Feb 2014 14:59:41 +0000 In-Reply-To: <1392397809-13255-1-git-send-email-julien.grall@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: xen-devel@lists.xenproject.org, tim@xen.org, george.dunlap@citrix.com, stefano.stabellini@citrix.com List-Id: xen-devel@lists.xenproject.org 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 > > --- > 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) )