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

[Peter Maydell]

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.

> +           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