| Message ID | 1392742577-3052-1-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
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; > }
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.
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.
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; }
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(+)