Patchwork linux-user: correct reboot()

login
register
mail settings
Submitter Laurent Vivier
Date Jan. 7, 2013, 8:30 p.m.
Message ID <1357590634-9768-1-git-send-email-laurent@vivier.eu>
Download mbox | patch
Permalink /patch/210112/
State New
Headers show

Comments

Laurent Vivier - Jan. 7, 2013, 8:30 p.m.
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(-)
Peter Maydell - Jan. 7, 2013, 8:42 p.m.
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
Laurent Vivier - Jan. 7, 2013, 8:51 p.m.
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
Peter Maydell - Jan. 7, 2013, 9:02 p.m.
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

Patch

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: