From: Hui Tang <[email protected]>
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")
Signed-off-by: Hui Tang <[email protected]>
---
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;
--
2.17.1
Ren Zhijie <[email protected]> writes:
> From: Hui Tang <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
>
> 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 <[email protected]> writes:
> > From: Hui Tang <[email protected]>
> >
> > 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
--
Kees Cook