Message ID | 1392397809-13255-1-git-send-email-julien.grall@linaro.org |
---|---|
State | Superseded, archived |
Headers | show |
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 >
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) )
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.
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.
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,
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.
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.
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) )
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(-)