Message-ID: <8D545969-2EFA-419A-B988-74AD0C26020C@kernel.org>
Date: Tue, 24 Sep 2024 14:37:13 -0700
From: Kees Cook <kees@...nel.org>
To: "Eric W. Biederman" <ebiederm@...ssion.com>,
 Tycho Andersen <tycho@...ho.pizza>
CC: Alexander Viro <viro@...iv.linux.org.uk>,
 Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
 Jeff Layton <jlayton@...nel.org>, Chuck Lever <chuck.lever@...cle.com>,
 Alexander Aring <alex.aring@...il.com>, linux-fsdevel@...r.kernel.org,
 linux-mm@...ck.org, linux-kernel@...r.kernel.org,
 Tycho Andersen <tandersen@...flix.com>,
 Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>,
 Aleksa Sarai <cyphar@...har.com>
Subject: Re: [RFC] exec: add a flag for "reasonable" execveat() comm



On September 24, 2024 10:39:35 AM PDT, "Eric W. Biederman" <ebiederm@...ssion.com> wrote:
>Tycho Andersen <tycho@...ho.pizza> writes:
>
>> From: Tycho Andersen <tandersen@...flix.com>
>>
>> Zbigniew mentioned at Linux Plumber's that systemd is interested in
>> switching to execveat() for service execution, but can't, because the
>> contents of /proc/pid/comm are the file descriptor which was used,
>> instead of the path to the binary. This makes the output of tools like
>> top and ps useless, especially in a world where most fds are opened
>> CLOEXEC so the number is truly meaningless.

And just to double check: systemd's use would be entirely cosmetic, yes?

>>
>> This patch adds an AT_ flag to fix up /proc/pid/comm to instead be the
>> contents of argv[0], instead of the fdno.
>
>The kernel allows prctl(PR_SET_NAME, ...)  without any permission
>checks so adding an AT_ flat to use argv[0] instead of the execed
>filename seems reasonable.
>
>Maybe the flag should be called AT_NAME_ARGV0.

If we add an AT flag I like this name.

>
>
>That said I am trying to remember why we picked /dev/fd/N, as the
>filename.
>
>My memory is that we couldn't think of anything more reasonable to use.
>Looking at commit 51f39a1f0cea ("syscalls: implement execveat() system
>call") unfortunately doesn't clarify anything for me, except that
>/dev/fd/N was a reasonable choice.
>
>I am thinking the code could reasonably try:
>	get_fs_root_rcu(current->fs, &root);
>	path = __d_path(file->f_path, root, buf, buflen);
>
>To see if a path to the file from the current root directory can be
>found.  For files that are not reachable from the current root the code
>still need to fallback to /dev/fd/N.
>
>Do you think you can investigate that and see if that would generate
>a reasonable task->comm?
>
>If for no other reason than because it would generate a usable result
>for #! scripts, without /proc mounted.
>
>
>It looks like a reasonable case can be made that while /dev/fd/N is
>a good path for interpreters, it is never a good choice for comm,
>so perhaps we could always use argv[0] if the fdpath is of the
>form /dev/fd/N.

I haven't had a chance to go look closely yet, but this was the same thought I had when I first read this RFC. Nobody really wants a dev path in comm. Can we do this unconditionally? (And if argv0 is empty, use dev path...)

>All of that said I am not a fan of the implementation below as it has
>the side effect of replacing /dev/fd/N with a filename that is not
>usable by #! interpreters.  So I suggest an implementation that affects
>task->comm and not brpm->filename.

Also agreed. There is already enough fiddly usage of the bprm filename/interpreter/fdpath members -- the argv0 stuff should be distinct. Perhaps store a pointer to argv0 during arg copy? I need to go look but I'm still AFK/OoO...

-Kees

-- 
Kees Cook

Powered by blists - more mailing lists