[Qemu-devel] [Patch] linux-user/syscall.c - don't add GUEST_BASE to NULL pointer

[Qemu-devel] [Patch] linux-user/syscall.c - don't add GUEST_BASE to NULL pointer

[Jan-Simon Möller]

This patch fixes the mount call. GUEST_BASE shouldn't be added to a NULL pointer on arg5 .
failing call: 
mount("rootfs", "/", 0x47a78, MS_MGC_VAL|MS_REMOUNT, 0x10000) = -1 EFAULT (Bad address)

correct call:
mount("rootfs", "/", 0x37ab0, MS_MGC_VAL|MS_REMOUNT, NULL) = 0


Signed-off-by:  Jan-Simon Möller  <dl9pf@gmx.de>
---
 linux-user/syscall.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 673eed4..5b2ec4f 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4445,12 +4445,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                        p3 = lock_user_string(arg3);
                         if (!p || !p2 || !p3)
                             ret = -TARGET_EFAULT;
-                        else
+                        else {
                             /* FIXME - arg5 should be locked, but it isn't clear how to
                              * do that since it's not guaranteed to be a NULL-terminated
                              * string.
                              */
-                            ret = get_errno(mount(p, p2, p3, (unsigned long)arg4, g2h(arg5)));
+                            if ( ! arg5 )
+                                ret = get_errno(mount(p, p2, p3, (unsigned long)arg4, NULL));
+                            else
+                                ret = get_errno(mount(p, p2, p3, (unsigned long)arg4, g2h(arg5)));
+                        }
                         unlock_user(p, arg1, 0);
                         unlock_user(p2, arg2, 0);
                         unlock_user(p3, arg3, 0);

Re: [Qemu-devel] [Patch] linux-user/syscall.c - don't add GUEST_BASE to NULL pointer

[Jan-Simon Möller]

Thinking a bit more about this, I wonder if g2h(x) shouldn't itself always 
return NULL on x = NULL ? 

Something like:

Signed-off-by:  Jan-Simon Möller  <dl9pf@gmx.de>

diff --git a/cpu-all.h b/cpu-all.h
index 1a6a812..631f678 100644
--- a/cpu-all.h
+++ b/cpu-all.h
@@ -633,7 +633,7 @@ extern int have_guest_base;
 #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) ( !x ? NULL:((void *)((unsigned long)(x) + GUEST_BASE)))
 #define h2g(x) ({ \
     unsigned long __ret = (unsigned long)(x) - GUEST_BASE; \
     /* Check if given address fits target address space */ \


I read the comment above, but before replacing it in user-mode (if possible), 
we should fix it ;) .


Best,
Jan-Simon

Re: [Qemu-devel] [Patch] linux-user/syscall.c - don't add GUEST_BASE to NULL pointer

[Riku Voipio]

On Wed, Aug 26, 2009 at 01:37:48AM +0200, Jan-Simon Möller wrote:
> Thinking a bit more about this, I wonder if g2h(x) shouldn't itself always 
> return NULL on x = NULL ? 

I agree this seems like a a better idea than modifying the users of g2h.

> Something like:
> 
> Signed-off-by:  Jan-Simon Möller  <dl9pf@gmx.de>
> 
> diff --git a/cpu-all.h b/cpu-all.h
> index 1a6a812..631f678 100644
> --- a/cpu-all.h
> +++ b/cpu-all.h
> @@ -633,7 +633,7 @@ extern int have_guest_base;
>  #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) ( !x ? NULL:((void *)((unsigned long)(x) + GUEST_BASE)))
>  #define h2g(x) ({ \
>      unsigned long __ret = (unsigned long)(x) - GUEST_BASE; \
>      /* Check if given address fits target address space */ \
> 
> 
> I read the comment above, but before replacing it in user-mode (if possible), 
> we should fix it ;) .
> 
> 
> Best,
> Jan-Simon
> 
> 
> 

Re: [Qemu-devel] [Patch] linux-user/syscall.c - don't add GUEST_BASE to NULL pointer

[Jan-Simon Möller]

Am Mittwoch 26 August 2009 15:40:43 schrieb Riku Voipio:
> On Wed, Aug 26, 2009 at 01:37:48AM +0200, Jan-Simon Möller wrote:
> > Thinking a bit more about this, I wonder if g2h(x) shouldn't itself
> > always return NULL on x = NULL ?
>
> I agree this seems like a a better idea than modifying the users of g2h.
>
Ok, then please apply to master.

Best,
Jan-Simon

Re: [Qemu-devel] [Patch] linux-user/syscall.c - don't add GUEST_BASE to NULL pointer

[Jan-Simon Möller]

Am Mittwoch 26 August 2009 15:40:43 schrieb Riku Voipio:
> On Wed, Aug 26, 2009 at 01:37:48AM +0200, Jan-Simon Möller wrote:
> > Thinking a bit more about this, I wonder if g2h(x) shouldn't itself
> > always return NULL on x = NULL ?
>
> I agree this seems like a a better idea than modifying the users of g2h.
>
> > Something like:
> >
> > Signed-off-by:  Jan-Simon Möller  <dl9pf@gmx.de>
> >
> > diff --git a/cpu-all.h b/cpu-all.h
> > index 1a6a812..631f678 100644
> > --- a/cpu-all.h
> > +++ b/cpu-all.h
> > @@ -633,7 +633,7 @@ extern int have_guest_base;
> >  #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) ( !x ? NULL:((void *)((unsigned long)(x) + GUEST_BASE)))
> >  #define h2g(x) ({ \
> >      unsigned long __ret = (unsigned long)(x) - GUEST_BASE; \
> >      /* Check if given address fits target address space */ \
> >
> >

Take the first patch for syscall.c / mount .

Unfortunately, the above one has side-effects where functions rely on a 
shifted NULL pointer ... 

Best,
Jan-Simon