[Qemu-devel] [PATCH] cpu-all.h: Don't accidentally sign extend in g2h()

[Qemu-devel] [PATCH] cpu-all.h: Don't accidentally sign extend in g2h()

[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))
 
 #if HOST_LONG_BITS <= TARGET_VIRT_ADDR_SPACE_BITS
 #define h2g_valid(x) 1
-- 
1.7.1

Re: [Qemu-devel] [PATCH] cpu-all.h: Don't accidentally sign extend in g2h()

[Andreas Färber]

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


-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] cpu-all.h: Don't accidentally sign extend in g2h()

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] cpu-all.h: Don't accidentally sign extend in g2h()

[Anthony Liguori]

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