[Qemu-devel] [PATCH] linux-user: correct reboot()

[Qemu-devel] [PATCH] linux-user: correct reboot()

[Laurent Vivier]

According to man reboot(2), the 4th argument is only used with
LINUX_REBOOT_CMD_RESTART2. In other cases, trying to convert
the value can generate EFAULT.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3167a87..730e428 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -101,6 +101,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #include <linux/fb.h>
 #include <linux/vt.h>
 #include <linux/dm-ioctl.h>
+#include <linux/reboot.h>
 #include "linux_loop.h"
 #include "cpu-uname.h"
 
@@ -6415,10 +6416,15 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
     case TARGET_NR_reboot:
-        if (!(p = lock_user_string(arg4)))
-            goto efault;
-        ret = reboot(arg1, arg2, arg3, p);
-        unlock_user(p, arg4, 0);
+        if (arg3 == LINUX_REBOOT_CMD_RESTART2) {
+           /* arg4 must be ignored in all other cases */
+           if (!(p = lock_user_string(arg4)))
+              goto efault;
+           ret = reboot(arg1, arg2, arg3, p);
+           unlock_user(p, arg4, 0);
+        } else {
+           ret = reboot(arg1, arg2, arg3, (void*)(unsigned long)arg4);
+        }
         break;
 #ifdef TARGET_NR_readdir
     case TARGET_NR_readdir:
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] linux-user: correct reboot()

[Peter Maydell]

On 7 January 2013 20:30, Laurent Vivier <laurent@vivier.eu> wrote:
> According to man reboot(2), the 4th argument is only used with
> LINUX_REBOOT_CMD_RESTART2. In other cases, trying to convert
> the value can generate EFAULT.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/syscall.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 3167a87..730e428 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -101,6 +101,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
>  #include <linux/fb.h>
>  #include <linux/vt.h>
>  #include <linux/dm-ioctl.h>
> +#include <linux/reboot.h>
>  #include "linux_loop.h"
>  #include "cpu-uname.h"
>
> @@ -6415,10 +6416,15 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>          break;
>  #endif
>      case TARGET_NR_reboot:
> -        if (!(p = lock_user_string(arg4)))
> -            goto efault;
> -        ret = reboot(arg1, arg2, arg3, p);
> -        unlock_user(p, arg4, 0);
> +        if (arg3 == LINUX_REBOOT_CMD_RESTART2) {
> +           /* arg4 must be ignored in all other cases */
> +           if (!(p = lock_user_string(arg4)))
> +              goto efault;

Coding style requires braces; please use checkpatch.pl.

> +           ret = reboot(arg1, arg2, arg3, p);
> +           unlock_user(p, arg4, 0);
> +        } else {
> +           ret = reboot(arg1, arg2, arg3, (void*)(unsigned long)arg4);

I don't think we should pass arg4 in this case. It's a pointer, so it's
definitely wrong to pass a pointer we haven't converted somehow.
Just passing NULL would be better, I  think; that will be safe and
make it reasonably obvious we need to fix something if the kernel
ever for some reason adds a new command that takes an argument.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: correct reboot()

[Laurent Vivier]

Le lundi 07 janvier 2013 à 20:42 +0000, Peter Maydell a écrit :
> On 7 January 2013 20:30, Laurent Vivier <laurent@vivier.eu> wrote:
> > According to man reboot(2), the 4th argument is only used with
> > LINUX_REBOOT_CMD_RESTART2. In other cases, trying to convert
> > the value can generate EFAULT.
> >
> > Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> > ---
> >  linux-user/syscall.c |   14 ++++++++++----
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 3167a87..730e428 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -101,6 +101,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
> >  #include <linux/fb.h>
> >  #include <linux/vt.h>
> >  #include <linux/dm-ioctl.h>
> > +#include <linux/reboot.h>
> >  #include "linux_loop.h"
> >  #include "cpu-uname.h"
> >
> > @@ -6415,10 +6416,15 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> >          break;
> >  #endif
> >      case TARGET_NR_reboot:
> > -        if (!(p = lock_user_string(arg4)))
> > -            goto efault;
> > -        ret = reboot(arg1, arg2, arg3, p);
> > -        unlock_user(p, arg4, 0);
> > +        if (arg3 == LINUX_REBOOT_CMD_RESTART2) {
> > +           /* arg4 must be ignored in all other cases */
> > +           if (!(p = lock_user_string(arg4)))
> > +              goto efault;
> 
> Coding style requires braces; please use checkpatch.pl.

Yes, sorry for that.

> 
> > +           ret = reboot(arg1, arg2, arg3, p);
> > +           unlock_user(p, arg4, 0);
> > +        } else {
> > +           ret = reboot(arg1, arg2, arg3, (void*)(unsigned long)arg4);
> 
> I don't think we should pass arg4 in this case. It's a pointer, so it's
> definitely wrong to pass a pointer we haven't converted somehow.
> Just passing NULL would be better, I  think; that will be safe and
> make it reasonably obvious we need to fix something if the kernel
> ever for some reason adds a new command that takes an argument.

Yes, but in the traces I have, arg4 is 1. Can we accept to loose it ?

Regards,
Laurent

-- 
"Just play. Have fun. Enjoy the game."
- Michael Jordan

Re: [Qemu-devel] [PATCH] linux-user: correct reboot()

[Peter Maydell]

On 7 January 2013 20:51, Laurent Vivier <Laurent@vivier.eu> wrote:
> Le lundi 07 janvier 2013 à 20:42 +0000, Peter Maydell a écrit :
>> On 7 January 2013 20:30, Laurent Vivier <laurent@vivier.eu> wrote:
>> > +           ret = reboot(arg1, arg2, arg3, (void*)(unsigned long)arg4);
>>
>> I don't think we should pass arg4 in this case. It's a pointer, so it's
>> definitely wrong to pass a pointer we haven't converted somehow.
>> Just passing NULL would be better, I  think; that will be safe and
>> make it reasonably obvious we need to fix something if the kernel
>> ever for some reason adds a new command that takes an argument.
>
> Yes, but in the traces I have, arg4 is 1. Can we accept to loose it ?

That will be because the syscall is just picking up whatever random
junk happens to be in the register than arg4 gets passed in. If you
look at the kernel sources:
http://lxr.linux.no/#linux+v3.7.1/kernel/sys.c#L425
'arg' is ignored for every other command.

-- PMM

[Qemu-devel] [PATCH][v2] linux-user: correct reboot()

[Laurent Vivier]

According to man reboot(2), the 4th argument is only used with
LINUX_REBOOT_CMD_RESTART2. In other cases, trying to convert
the value can generate EFAULT.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: Set arg4 to NULL when arg3 != LINUX_REBOOT_CMD_RESTART2
    use get_errno()
    check patch checkpatch.pl
 linux-user/syscall.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3167a87..24c70e3 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -101,6 +101,7 @@ int __clone2(int (*fn)(void *), void *child_stack_base,
 #include <linux/fb.h>
 #include <linux/vt.h>
 #include <linux/dm-ioctl.h>
+#include <linux/reboot.h>
 #include "linux_loop.h"
 #include "cpu-uname.h"
 
@@ -6415,10 +6416,17 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         break;
 #endif
     case TARGET_NR_reboot:
-        if (!(p = lock_user_string(arg4)))
-            goto efault;
-        ret = reboot(arg1, arg2, arg3, p);
-        unlock_user(p, arg4, 0);
+        if (arg3 == LINUX_REBOOT_CMD_RESTART2) {
+           /* arg4 must be ignored in all other cases */
+           p = lock_user_string(arg4);
+           if (!p) {
+              goto efault;
+           }
+           ret = get_errno(reboot(arg1, arg2, arg3, p));
+           unlock_user(p, arg4, 0);
+        } else {
+           ret = get_errno(reboot(arg1, arg2, arg3, NULL));
+        }
         break;
 #ifdef TARGET_NR_readdir
     case TARGET_NR_readdir:
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH][v2] linux-user: correct reboot()

[Peter Maydell]

On 7 January 2013 21:40, Laurent Vivier <laurent@vivier.eu> wrote:
> According to man reboot(2), the 4th argument is only used with
> LINUX_REBOOT_CMD_RESTART2. In other cases, trying to convert
> the value can generate EFAULT.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

Re: [Qemu-devel] [PATCH][v2] linux-user: correct reboot()

[Laurent Vivier]

Le lundi 07 janvier 2013 à 21:46 +0000, Peter Maydell a écrit :
> On 7 January 2013 21:40, Laurent Vivier <laurent@vivier.eu> wrote:
> > According to man reboot(2), the 4th argument is only used with
> > LINUX_REBOOT_CMD_RESTART2. In other cases, trying to convert
> > the value can generate EFAULT.
> >
> > Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

ping ?

Laurent

-- 
"Just play. Have fun. Enjoy the game."
- Michael Jordan