| Message ID | 1357590634-9768-1-git-send-email-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
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
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
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
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:
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(-)