| Message ID | 1331303600-30715-1-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Commit | 8d9dde9429d2b5b513bf92d94a257a00ea2da1bf |
| Headers | show |
Am 09.03.2012 15:33, schrieb Peter Maydell: > Cast the argument of the g2h() macro to a target_ulong so that > it isn't accidentally sign-extended if it is a signed 32 bit > type and long is a 64 bit type. In particular, this fixes a > bug where it would return the wrong value for 32 bit guests > on 64 bit hosts when passed in one of the arg* values from > do_syscall() [which are all abi_long and thus signed types]. > This could result in spurious failure of mlock(), among others. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > This should be committed before Alex's patch to make mmap allocate > downwards (http://patchwork.ozlabs.org/patch/144476/) because that > hugely increases the chances that g2h will get passed a pointer > that has the high bit set. > > cpu-all.h | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/cpu-all.h b/cpu-all.h > index 80e6d42..a174532 100644 > --- a/cpu-all.h > +++ b/cpu-all.h > @@ -197,7 +197,7 @@ extern unsigned long reserved_va; > #endif > > /* All direct uses of g2h and h2g need to go away for usermode softmmu. */ > -#define g2h(x) ((void *)((unsigned long)(x) + GUEST_BASE)) > +#define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + GUEST_BASE)) So *only* for a 32-bit guest does this cast from signed int to unsigned int and then to unsigned long, avoiding the sign extension on 64-bit host. For 64-bit guests it remains as broken as before. Commit message could be clearer. Reviewed-by: Andreas Färber <afaerber@suse.de> Note that unsigned long would be wrong for Win64 (where we don't currently have any user emulation using this macro). uintptr_t would be cleaner. Andreas > > #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS > #define h2g_valid(x) 1
On 9 March 2012 14:55, Andreas Färber <afaerber@suse.de> wrote: > Am 09.03.2012 15:33, schrieb Peter Maydell: >> Cast the argument of the g2h() macro to a target_ulong so that >> it isn't accidentally sign-extended if it is a signed 32 bit >> type and long is a 64 bit type. In particular, this fixes a >> bug where it would return the wrong value for 32 bit guests >> on 64 bit hosts when passed in one of the arg* values from >> do_syscall() [which are all abi_long and thus signed types]. >> This could result in spurious failure of mlock(), among others. > So *only* for a 32-bit guest does this cast from signed int to unsigned > int and then to unsigned long, avoiding the sign extension on 64-bit > host. For 64-bit guests it remains as broken as before. Commit message > could be clearer. The commit message is only claiming to fix a bug "for 32 bit guests on 64 bit hosts" -- that seemed fairly clear to me when I wrote it, and indeed it's only the 32-on-64 behaviour which the patch changes. 64 bit guests on 64 bit hosts remain OK because the value is in a signed 64 bit integer which is cast to an unsigned 64 bit integer (twice). 64 bit guests on 32 bit hosts may or may not be broken for other reasons, but this change doesn't alter the behaviour of this macro for them either. > Note that unsigned long would be wrong for Win64 (where we don't > currently have any user emulation using this macro). > uintptr_t would be cleaner. Probably true, but there are a lot of 'unsigned long's lurking in cpu-all.h, so that would be a separate cleanup patch. -- PMM
On 03/09/2012 08:33 AM, Peter Maydell wrote: > Cast the argument of the g2h() macro to a target_ulong so that > it isn't accidentally sign-extended if it is a signed 32 bit > type and long is a 64 bit type. In particular, this fixes a > bug where it would return the wrong value for 32 bit guests > on 64 bit hosts when passed in one of the arg* values from > do_syscall() [which are all abi_long and thus signed types]. > This could result in spurious failure of mlock(), among others. > > Signed-off-by: Peter Maydell<peter.maydell@linaro.org> > --- Applied. Thanks. Regards, Anthony Liguori > This should be committed before Alex's patch to make mmap allocate > downwards (http://patchwork.ozlabs.org/patch/144476/) because that > hugely increases the chances that g2h will get passed a pointer > that has the high bit set. > > cpu-all.h | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/cpu-all.h b/cpu-all.h > index 80e6d42..a174532 100644 > --- a/cpu-all.h > +++ b/cpu-all.h > @@ -197,7 +197,7 @@ extern unsigned long reserved_va; > #endif > > /* All direct uses of g2h and h2g need to go away for usermode softmmu. */ > -#define g2h(x) ((void *)((unsigned long)(x) + GUEST_BASE)) > +#define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + GUEST_BASE)) > > #if HOST_LONG_BITS<= TARGET_VIRT_ADDR_SPACE_BITS > #define h2g_valid(x) 1
diff --git a/cpu-all.h b/cpu-all.h index 80e6d42..a174532 100644 --- a/cpu-all.h +++ b/cpu-all.h @@ -197,7 +197,7 @@ extern unsigned long reserved_va; #endif /* All direct uses of g2h and h2g need to go away for usermode softmmu. */ -#define g2h(x) ((void *)((unsigned long)(x) + GUEST_BASE)) +#define g2h(x) ((void *)((unsigned long)(target_ulong)(x) + GUEST_BASE)) #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS #define h2g_valid(x) 1
Cast the argument of the g2h() macro to a target_ulong so that it isn't accidentally sign-extended if it is a signed 32 bit type and long is a 64 bit type. In particular, this fixes a bug where it would return the wrong value for 32 bit guests on 64 bit hosts when passed in one of the arg* values from do_syscall() [which are all abi_long and thus signed types]. This could result in spurious failure of mlock(), among others. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- This should be committed before Alex's patch to make mmap allocate downwards (http://patchwork.ozlabs.org/patch/144476/) because that hugely increases the chances that g2h will get passed a pointer that has the high bit set. cpu-all.h | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)