Re: [RFC] exec: add a flag for "reasonable" execveat() comm

[Tycho Andersen <tycho@tycho.pizza>]

On Tue, Sep 24, 2024 at 02:37:13PM -0700, Kees Cook wrote:
> 
> 
> On September 24, 2024 10:39:35 AM PDT, "Eric W. Biederman" <ebiederm@xmission.com> wrote:
> >Tycho Andersen <tycho@tycho.pizza> writes:
> >
> >> From: Tycho Andersen <tandersen@netflix.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?

I think it's not really systemd, but their concern for admins looking
at `ps` and being confused by "4 is using lots of CPU". IIUC systemd
won't actually use the value at all. Zbigniew can confirm though.

> >>
> >> 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.

+1

> >
> >
> >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...)

We can, I was just worried about the behavior change. But it seems we
are all in violent agreement that the current behavior isn't very
good, so maybe it's fine to change.

> >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...

Yeah, on second thought we could do something like:

diff --git a/fs/exec.c b/fs/exec.c
index 36434feddb7b..a45ea270cc43 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1416,7 +1416,10 @@ int begin_new_exec(struct linux_binprm * bprm)
                set_dumpable(current->mm, SUID_DUMP_USER);

        perf_event_exec();
-       __set_task_comm(me, kbasename(bprm->filename), true);
+       if (needs_comm_fixup)
+               __set_task_comm(me, argv0, true);
+       else
+               __set_task_comm(me, kbasename(bprm->filename), true);

        /* An exec changes our domain. We are no longer part of the thread
           group */

and then we don't need to mess with bprm at all. Seems much cleaner. I
will see about the

	get_fs_root_rcu(current->fs, &root);
	path = __d_path(file->f_path, root, buf, buflen);

that Eric suggested and how that works with the above.

Tycho