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

[Zbigniew Jędrzejewski-Szmek]

On Tue, Sep 24, 2024 at 12:39:35PM -0500, Eric W. Biederman wrote:
> Tycho Andersen <tycho@tycho.pizza> writes:
> 
> > From: Tycho Andersen <tandersen@xxxxxxxxxxx>
> >
> > 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.
> >
> > This patch adds an AT_ flag to fix up /proc/pid/comm to instead be the
> > contents of argv[0], instead of the fdno.

I tried this version (with a local modification to drop the flag and
enable the new codepath if get_user_arg_ptr(argv, 0) returns nonnull
as suggested later in the thread), and it seems to work as expected.
In particular, 'pgrep' finds for the original name in case of
symlinks.

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

Hmm, I don't understand this. /dev/fd/ would not generally contain an
open fd for the original binary. It only would if the caller uses
fexecve with an fd opened without O_CLOEXEC, but then it'd be
something like /dev/fd/3 or /dev/fd/4 and the callee would be confused
by having an extra fd, so except for some specialed cases, the caller
should always use O_CLOEXEC.

With this patch:
$ sudo ln -sv /bin/sleep /usr/local/bin/sleep-link
$ sudo systemd-run sleep-link 10000

$ sudo strace -f -e execve,execveat -p 1
...
[pid  1200] execve("/proc/self/fd/9", ["/usr/lib/systemd/systemd-executo"..., "--deserialize", "150", "--log-level", "info", "--log-target", "journal-or-kmsg"], 0x7ffe97b98178 /* 3 vars */) = 0
[pid  1200] execveat(4, "", ["/usr/local/bin/sleep-link", "10000"], 0xd8edf70 /* 9 vars */, AT_EMPTY_PATH) = 0
^C

$ pgrep sleep-link
1200

$ sudo ls -l /proc/1200/fd
total 0
lr-x------ 1 root root 64 Oct  2 17:13 0 -> /dev/null
lrwx------ 1 root root 64 Oct  2 17:13 1 -> 'socket:[8585]'
lrwx------ 1 root root 64 Oct  2 17:13 2 -> 'socket:[8585]'

$ head -n1 /proc/1200/{comm,status,stat}
==> /proc/1200/comm <==
sleep-link
==> /proc/1200/status <==
Name:   sleep-link
==> /proc/1200/stat <==
1200 (sleep-link) ...

This all looks good.

Zbyszek