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 <address@hidden> 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 <address@hidden>
> > ---
> >  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