Message-ID: <87jzcxv227.fsf@email.froward.int.ebiederm.org>
Date: Wed, 20 Nov 2024 18:54:40 -0600
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Kees Cook <kees@...nel.org>,  linux-kernel@...r.kernel.org,  Alexander
 Viro <viro@...iv.linux.org.uk>,  Christophe JAILLET
 <christophe.jaillet@...adoo.fr>,  Dan Carpenter
 <dan.carpenter@...aro.org>,  Nir Lichtman <nir@...htman.org>,
  syzbot+03e1af5c332f7e0eb84b@...kaller.appspotmail.com,  Tycho Andersen
 <tandersen@...flix.com>,  Vegard Nossum <vegard.nossum@...cle.com>,
  Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>
Subject: Re: [GIT PULL] execve updates for v6.13-rc1

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Tue, 19 Nov 2024 at 09:00, Kees Cook <kees@...nel.org> wrote:
>>
>> - exec: Use argv[0] for "comm" with AT_EMPTY_PATH (Tycho Andersen,
>>   Dan Carpenter, Nir Lichtman)
>
> Ugh. I *really* despise this one.
>
> People: we *have* a filename. It's right there in the dentry. Which is
> right there as bprm->file->f_dentry.dentry.

__set_task_comm cannot be called with bprm->file->f_dentry
unconditionally.  That will break userspace.  On at least debian using
/etc/alternatives there are a lot of symlinks to executables.

For example if I run "vi", two symlinks are followed:
/usr/bin/vi -> /etc/alternatives/vi
/etc/alternatives/vi -> usr/bin/vim.nox

Seeing "vim.nox" in ps instead "vi" would be weird and break tooks like
psgrep.

https://lore.kernel.org/all/Zv1OayMEmLP2kjhj@kawka3.in.waw.pl/

The reason bprm->file->f_dentry.dentry was abandoned were concerns
about breaking userspace.


Limited to just the binaries that systemd wants to call.  I don't know
if symlinks to binaries matters.  At a quick skim I couldn't see
anything but *shrug*.


> And that's actually going to match the actual execcutable, unlike, for
> example, argv[0], which can be filled in with random data.
>
>
> *AND* we don't need any silly and expensive get_user_arg_ptr() and
> strndup_user() copy for it, which does that user access twice.
>
> And no, we shouldn't fall back to the horrid thing that bprm->fdpath
> does either. That's the thing that you apparently thought was too ugly
> to use, and literally the *only* use of it was for this case.

bprm->fdpath is for passing the pathname of the script to an interpreter
like /bin/sh.  That task->comm wind up being filled from it, is a bit
of a surprise.

> The reason that code existed at all was to generate a filename, and
> because we didn't use to have access to the 'bprm->file' back in the
> days.
>
> But that was changed by commit 978ffcbf00d8 ("execve: open the
> executable file before doing anything else").

That should work for binfmt_script.  I don't know which solution
is less racy, and less prone to problems in the binfmt_script case.
No one has complained so I have been assuming that case works just
fine.

> So I really really think that what this code *should* have done is
>
>  - get rid of fdpath that you made pointless by not using it for 'comm[]'

Again binfmt_script still uses it.

I can assure you there was no thought at all given to task->comm when
the contents of bprm->fdpath were chosen.  Which is why there is
a problem now.

task->comm is for people.  bprm->fdpath is for programs.

>  - teach the code to use the dentry name instead

Which is fine for task->comm, when the pathname is NULL. Assuming it
actually makes things usable for userspace.

That doesn't work for bprm->fdpath with becomes bprm->interp.  That
would require using d_path.

Eric

Powered by blists - more mailing lists