| Message ID | 20220920120812.231417-1-renzhijie2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | exec: Force binary name when argv is empty | expand |
Ren Zhijie <renzhijie2@huawei.com> writes: > From: Hui Tang <tanghui20@huawei.com> > > First run './execv-main execv-child', there is empty in 'COMMAND' column > when run 'ps -u'. > > USER PID %CPU %MEM VSZ RSS TTY [...] TIME COMMAND > root 368 0.3 0.0 4388 764 ttyS0 0:00 ./execv-main > root 369 0.6 0.0 4520 812 ttyS0 0:00 > > The program 'execv-main' as follows: > > int main(int argc, char **argv) > { > char *execv_argv[] = {NULL}; > pid_t pid = fork(); > > if (pid == 0) { > execv(argv[1], execv_argv); > } else if (pid > 0) { > wait(NULL); > } > return 0; > } > > So replace empty string ("") added with the name of binary > when calling execve with a NULL argv. I do not understand the point of this patch. The command name is allowed to be anything. By convention it is the name of the binary but that is not required. For login shells it is always something else. The practical problem that commit dcd46d897adb ("exec: Force single empty string when argv is empty") addresses is that a NULL argv[0] is not expected by programs, and can be used to trigger bugs in those programs. Especially suid programs. The actual desired behavior is to simply have execve fail in that case. Unfortunately there are a few existing programs that depend on running that way, so we could not have such exec's fail. For a rare case that should essentially never happen why make it friendlier to use? Why not fix userspace to add the friendly name instead of the kernel? Unless there is a good reason for it, it would be my hope that in a couple of years all of the userspace programs that trigger the warning when they start up could be fixed, and we could have execve start failing in those cases. Eric > > Fixes: dcd46d897adb ("exec: Force single empty string when argv is empty") > Signed-off-by: Hui Tang <tanghui20@huawei.com> > --- > fs/exec.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 939d76e23935..7d1909a89a57 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -494,8 +494,8 @@ static int bprm_stack_limits(struct linux_binprm *bprm) > * signal to the parent that the child has run out of stack space. > * Instead, calculate it here so it's possible to fail gracefully. > * > - * In the case of argc = 0, make sure there is space for adding a > - * empty string (which will bump argc to 1), to ensure confused > + * In the case of argc = 0, make sure there is space for adding > + * bprm->filename (which will bump argc to 1), to ensure confused > * userspace programs don't start processing from argv[1], thinking > * argc can never be 0, to keep them from walking envp by accident. > * See do_execveat_common(). > @@ -1900,7 +1900,7 @@ static int do_execveat_common(int fd, struct filename *filename, > > retval = count(argv, MAX_ARG_STRINGS); > if (retval == 0) > - pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n", > + pr_warn_once("process '%s' launched '%s' with NULL argv: bprm->filename added\n", > current->comm, bprm->filename); > if (retval < 0) > goto out_free; > @@ -1929,13 +1929,13 @@ static int do_execveat_common(int fd, struct filename *filename, > goto out_free; > > /* > - * When argv is empty, add an empty string ("") as argv[0] to > + * When argv is empty, add bprm->filename as argv[0] to > * ensure confused userspace programs that start processing > * from argv[1] won't end up walking envp. See also > * bprm_stack_limits(). > */ > if (bprm->argc == 0) { > - retval = copy_string_kernel("", bprm); > + retval = copy_string_kernel(bprm->filename, bprm); > if (retval < 0) > goto out_free; > bprm->argc = 1;
On Tue, Sep 20, 2022 at 08:08:12PM +0800, Ren Zhijie wrote: > From: Hui Tang <tanghui20@huawei.com> > > First run './execv-main execv-child', there is empty in 'COMMAND' column > when run 'ps -u'. > > USER PID %CPU %MEM VSZ RSS TTY [...] TIME COMMAND > root 368 0.3 0.0 4388 764 ttyS0 0:00 ./execv-main > root 369 0.6 0.0 4520 812 ttyS0 0:00 > > The program 'execv-main' as follows: > > int main(int argc, char **argv) > { > char *execv_argv[] = {NULL}; > pid_t pid = fork(); > > if (pid == 0) { > execv(argv[1], execv_argv); > } else if (pid > 0) { > wait(NULL); > } > return 0; > } > > So replace empty string ("") added with the name of binary > when calling execve with a NULL argv. > > Fixes: dcd46d897adb ("exec: Force single empty string when argv is empty") I don't see the point, to be honest... You've passed BS argv to execve(), why would you expect anything pretty from ps(1)? IOW, where's the bug you are fixing?
On Tue, Sep 20, 2022 at 09:42:48AM -0500, Eric W. Biederman wrote: > Ren Zhijie <renzhijie2@huawei.com> writes: > > From: Hui Tang <tanghui20@huawei.com> > > > > First run './execv-main execv-child', there is empty in 'COMMAND' column > > when run 'ps -u'. > > > > USER PID %CPU %MEM VSZ RSS TTY [...] TIME COMMAND > > root 368 0.3 0.0 4388 764 ttyS0 0:00 ./execv-main > > root 369 0.6 0.0 4520 812 ttyS0 0:00 > > > > The program 'execv-main' as follows: > > > > int main(int argc, char **argv) > > { > > char *execv_argv[] = {NULL}; > > pid_t pid = fork(); > > > > if (pid == 0) { > > execv(argv[1], execv_argv); > > } else if (pid > 0) { > > wait(NULL); > > } > > return 0; > > } The correct fix is to userspace here: int main(int argc, char **argv) { - char *execv_argv[] = {NULL}; + char *execv_argv[] = { argv[1], NULL }; pid_t pid = fork(); if (pid == 0) { > [...] > For a rare case that should essentially never happen why make it > friendlier to use? Why not fix userspace to add the friendly name > instead of the kernel? > > Unless there is a good reason for it, it would be my hope that in > a couple of years all of the userspace programs that trigger > the warning when they start up could be fixed, and we could have > execve start failing in those cases. Agreed -- the goal is to help userspace fix how execve(2) is called. Speaking to the proposed patch, this idea was considered during the development of the ""-adding patch, with the basic outcome being that creating a _new_ behavior was not a good idea, and might cause more confusion. You can see the thread here: https://lore.kernel.org/all/202202021229.9681AD39B0@keescook/ -Kees
diff --git a/fs/exec.c b/fs/exec.c index 939d76e23935..7d1909a89a57 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -494,8 +494,8 @@ static int bprm_stack_limits(struct linux_binprm *bprm) * signal to the parent that the child has run out of stack space. * Instead, calculate it here so it's possible to fail gracefully. * - * In the case of argc = 0, make sure there is space for adding a - * empty string (which will bump argc to 1), to ensure confused + * In the case of argc = 0, make sure there is space for adding + * bprm->filename (which will bump argc to 1), to ensure confused * userspace programs don't start processing from argv[1], thinking * argc can never be 0, to keep them from walking envp by accident. * See do_execveat_common(). @@ -1900,7 +1900,7 @@ static int do_execveat_common(int fd, struct filename *filename, retval = count(argv, MAX_ARG_STRINGS); if (retval == 0) - pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n", + pr_warn_once("process '%s' launched '%s' with NULL argv: bprm->filename added\n", current->comm, bprm->filename); if (retval < 0) goto out_free; @@ -1929,13 +1929,13 @@ static int do_execveat_common(int fd, struct filename *filename, goto out_free; /* - * When argv is empty, add an empty string ("") as argv[0] to + * When argv is empty, add bprm->filename as argv[0] to * ensure confused userspace programs that start processing * from argv[1] won't end up walking envp. See also * bprm_stack_limits(). */ if (bprm->argc == 0) { - retval = copy_string_kernel("", bprm); + retval = copy_string_kernel(bprm->filename, bprm); if (retval < 0) goto out_free; bprm->argc = 1;