Re: [PATCHv6 1/3] syscalls,x86: implement execveat() system call

[David Drysdale]

On Thu, Nov 6, 2014 at 6:06 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Thu, Nov 6, 2014 at 8:07 AM, David Drysdale <drysdale@xxxxxxxxxx> wrote:
>> diff --git a/fs/exec.c b/fs/exec.c
>> index a2b42a98c743..800d232c17bb 100644
>> --- a/fs/exec.c
>> +++ b/fs/exec.c
>> @@ -1422,10 +1431,12 @@ static int exec_binprm(struct linux_binprm *bprm)
>>  /*
>>   * sys_execve() executes a new program.
>>   */
>> -static int do_execve_common(struct filename *filename,
>> -                               struct user_arg_ptr argv,
>> -                               struct user_arg_ptr envp)
>> +static int do_execveat_common(int fd, struct filename *filename,
>> +                             struct user_arg_ptr argv,
>> +                             struct user_arg_ptr envp,
>> +                             int flags)
>>  {
>> +       char *pathbuf = NULL;
>>         struct linux_binprm *bprm;
>>         struct file *file;
>>         struct files_struct *displaced;
>> @@ -1466,7 +1477,7 @@ static int do_execve_common(struct filename *filename,
>>         check_unsafe_exec(bprm);
>>         current->in_execve = 1;
>>
>> -       file = do_open_exec(filename);
>> +       file = do_open_execat(fd, filename, flags);
>>         retval = PTR_ERR(file);
>>         if (IS_ERR(file))
>>                 goto out_unmark;
>> @@ -1474,7 +1485,30 @@ static int do_execve_common(struct filename *filename,
>>         sched_exec();
>>
>>         bprm->file = file;
>> -       bprm->filename = bprm->interp = filename->name;
>> +       if (fd == AT_FDCWD || filename->name[0] == '/') {
>> +               bprm->filename = filename->name;
>> +       } else {
>> +               /* "/dev/fd/2147483647/" + filename->name */
>> +               int maxlen = 19 + strlen(filename->name);
>
> This should be 20 + strlen (to include the trailing NULL). However, I
> think this whole bit of code could be replaced with kasprintf...
>
>> +
>> +               pathbuf = kmalloc(maxlen, GFP_TEMPORARY);
>> +               if (!pathbuf) {
>> +                       retval = -ENOMEM;
>> +                       goto out_unmark;
>> +               }
>> +               if (filename->name[0] == '\0')
>> +                       sprintf(pathbuf, "/dev/fd/%d", fd);
>> +               else
>> +                       snprintf(pathbuf, maxlen,
>> +                                "/dev/fd/%d/%s", fd, filename->name);
>
> Maybe something like this? A bit messy, so maybe your original if/else
> would be more readable.
>
>     } else {
>         pathbuf = kasprintf("/dev/fd/%d%s%s", fd,
>                                        filename->name[0] == '\0' ? "" : "/",
>                                        filename->name[0] == '\0' ? ""
> : filename->name);
>         if (!pathbuf) {
>             retval = -ENOMEM;
>             goto out_unmark;
>         }
>     }

Ah, I didn't know about kasprintf(), that will make things much
easier -- thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/