Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Ren Zhijie <renzhijie2@huawei.com>
Cc:     <keescook@chromium.org>, <viro@zeniv.linux.org.uk>,
        <linux-mm@kvack.org>, <linux-fsdevel@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <tanghui20@huawei.com>
References: <20220920120812.231417-1-renzhijie2@huawei.com>
Date:   Tue, 20 Sep 2022 09:42:48 -0500
In-Reply-To: <20220920120812.231417-1-renzhijie2@huawei.com> (Ren Zhijie's
        message of "Tue, 20 Sep 2022 20:08:12 +0800")
Message-ID: <87sfkmyumv.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] exec: Force binary name when argv is empty
Precedence: bulk

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;