Re: [Qemu-devel] [PATCH] linux-user: add option to intercept execve() syscalls

[Petros Angelatos]

>> diff --git a/linux-user/main.c b/linux-user/main.c
>> index ee12035..5951279 100644
>> --- a/linux-user/main.c
>> +++ b/linux-user/main.c
>> @@ -79,6 +79,7 @@ static void usage(int exitcode);
>>
>>  static const char *interp_prefix = CONFIG_QEMU_INTERP_PREFIX;
>>  const char *qemu_uname_release;
>> +const char *qemu_execve_path;
>>
>>  /* XXX: on x86 MAP_GROWSDOWN only works if ESP <= address + 32, so
>>     we allocate a bigger stack. Need a better solution, for example
>> @@ -3828,6 +3829,11 @@ static void handle_arg_guest_base(const char *arg)
>>      have_guest_base = 1;
>>  }
>>
>> +static void handle_arg_execve(const char *arg)
>> +{
>> +    qemu_execve_path = strdup(arg);
>
> I think you can use the parameter just as an on/off switch and
> realpath(argv[0]) as qemu_execve_path.
>
> I don't see any reason to use other binary than the one in use.

This was my initial approach too, but argv[0] can be just the filename
like "qemu-arm-static". And while I could add extra logic to look this
up in the PATH, someone could run it from a completely different
location. Then I looked for a way to get the path of the current
executable but every platform has its own way of doing that and I
didn't want to add all these cases.

https://stackoverflow.com/questions/1023306/finding-current-executables-path-without-proc-self-exe

>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 0cbace4..d0b5442 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -5854,6 +5854,109 @@ static target_timer_t get_timer_id(abi_long arg)
>>      return timerid;
>>  }
>>
>> +#define BINPRM_BUF_SIZE 128
>
> This is defined in <linux/binfmts.h>

Got it, I'll add this header and remove the definition.

>
>> +/* qemu_execve() Must return target values and target errnos. */
>> +static abi_long qemu_execve(char *filename, char *argv[],
>> +                  char *envp[])
>> +{
>> +    char *i_arg = NULL, *i_name = NULL;
>> +    char **new_argp;
>> +    int argc, fd, ret, i, offset = 3;
>> +    char *cp;
>> +    char buf[BINPRM_BUF_SIZE];
>> +
>> +    for (argc = 0; argv[argc] != NULL; argc++) {
>> +        /* nothing */ ;
>> +    }
>> +
>> +    fd = open(filename, O_RDONLY);
>> +    if (fd == -1) {
>> +        return -ENOENT;
>
> return -errno; ?

Will fix in v2

>> +    ret = read(fd, buf, BINPRM_BUF_SIZE);
>> +    if (ret == -1) {
>> +        close(fd);
>> +        return -ENOENT;
>
> return -errno; ?

Will fix in v2

>> +    }
>> +
>> +    close(fd);
>> +
>> +    /* adapted from the kernel
>> +     * 
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/binfmt_script.c
>> +     */
>> +    if ((buf[0] == '#') && (buf[1] == '!')) {
>
> what happens if read() < 2 ?

Hm, the easy option is for qemu_execve to return ENOEXEC or EIO.
Otherwise I could retry the read N times? I'm not sure how to handle
this if we don't want to return an error.

>> +    /* Copy the original arguments with offset */
>> +    for (i = 0; i < argc; i++) {
>> +        new_argp[i + offset] = argv[i];
>> +    }
>> +
>> +    new_argp[0] = strdup(qemu_execve_path);
>> +    new_argp[1] = strdup("-0");
>> +    new_argp[offset] = filename;
>> +    new_argp[argc + offset] = NULL;
>> +
>> +    if (i_name) {
>> +        new_argp[2] = i_name;
>> +        new_argp[3] = i_name;
>> +
>> +        if (i_arg) {
>> +            new_argp[4] = i_arg;
>> +        }
>> +    } else {
>> +        new_argp[2] = argv[0];
>> +    }
>> +
>> +    return get_errno(execve(qemu_execve_path, new_argp, envp));
>
> duplicate get_errno() with the caller.

I'll add the logic proposed bellow in this function and remove the
duplicate get_errno() from the caller.

>>  /* do_syscall() should always have a single exit point at the end so
>>     that actions, such as logging of syscall results, can be performed.
>>     All errnos that do_syscall() returns must be -TARGET_<errcode>. */
>> @@ -6113,7 +6216,13 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
>> arg1,
>>
>>              if (!(p = lock_user_string(arg1)))
>>                  goto execve_efault;
>> -            ret = get_errno(execve(p, argp, envp));
>> +
>> +            if (qemu_execve_path && *qemu_execve_path) {
>> +                ret = get_errno(qemu_execve(p, argp, envp));
>> +            } else {
>> +                ret = get_errno(execve(p, argp, envp));
>> +            }
>> +
>
> what do you think of:
>
>     ret = qemu_execve(p, argp, envp);
>
> and in qemu_execve():
>
>     if (qemu_execve_path == NULL || *qemu_execve_path == 0) {
>         return get_errno(execve(p, argp, envp));
>     }
>
> so all the logic is in the function.

Sounds good, I'll include this in v2

Since I'm new to this style of contribution I have a couple of
questions. Is it ok that I deleted part of the patch for my reply to
code review, or should I have replied inline without deleting
anything? Should I send v2 after we resolve all the issues here or
should I send a v2 with proposed fixes but lacking the ones pending
replies?

Thanks,
Petros