[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <202410141403.D8B6671@keescook>
Date: Mon, 14 Oct 2024 14:13:32 -0700
From: Kees Cook <kees@...nel.org>
To: Tycho Andersen <tycho@...ho.pizza>
Cc: Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>,
"Eric W. Biederman" <ebiederm@...ssion.com>,
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>,
Aleksa Sarai <cyphar@...har.com>
Subject: Re: [RFC] exec: add a flag for "reasonable" execveat() comm
On Wed, Oct 09, 2024 at 08:41:31AM -0600, Tycho Andersen wrote:
> On Wed, Oct 02, 2024 at 02:34:43PM +0000, Zbigniew Jędrzejewski-Szmek wrote:
> > On Tue, Sep 24, 2024 at 12:39:35PM -0500, Eric W. Biederman 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.
> > > >
> > > > 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.
>
> Here is a version that only affects /proc/pid/comm, without a flag. We
> still have to do the dance of keeping the user argv0 before actually
> doing __set_task_comm(), since we want to surface the resulting fault
> if people pass a bad argv0. Thoughts?
>
> Tycho
>
>
>
> diff --git a/fs/exec.c b/fs/exec.c
> index dad402d55681..61de8a71f316 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1416,7 +1416,16 @@ 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 fdpath was set, execveat() made up a path that will
> + * probably not be useful to admins running ps or similar.
> + * Let's fix it up to be something reasonable.
> + */
> + if (bprm->argv0)
> + __set_task_comm(me, kbasename(bprm->argv0), true);
> + else
> + __set_task_comm(me, kbasename(bprm->filename), true);
This isn't checking fdpath?
>
> /* An exec changes our domain. We are no longer part of the thread
> group */
> @@ -1566,9 +1575,30 @@ static void free_bprm(struct linux_binprm *bprm)
> if (bprm->interp != bprm->filename)
> kfree(bprm->interp);
> kfree(bprm->fdpath);
> + kfree(bprm->argv0);
> kfree(bprm);
> }
>
> +static int bprm_add_fixup_comm(struct linux_binprm *bprm, struct user_arg_ptr argv)
> +{
> + const char __user *p = get_user_arg_ptr(argv, 0);
> +
> + /*
> + * In keeping with the logic in do_execveat_common(), we say p == NULL
> + * => "" for comm.
> + */
> + if (!p) {
> + bprm->argv0 = kstrdup("", GFP_KERNEL);
> + return 0;
> + }
> +
> + bprm->argv0 = strndup_user(p, MAX_ARG_STRLEN);
> + if (bprm->argv0)
> + return 0;
> +
> + return -EFAULT;
> +}
I'd rather this logic got done in copy_strings() and to avoid duplicating
a copy for all exec users. I think it should be possible to just do
this, to find the __user char *:
diff --git a/fs/exec.c b/fs/exec.c
index 77364806b48d..e12fd706f577 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -642,6 +642,8 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
goto out;
}
}
+ if (argc == 0)
+ bprm->argv0 = str;
}
ret = 0;
out:
Once we get to begin_new_exec(), only if we need to do the work (fdpath
set), then we can do the strndup_user() instead of making every exec
hold a copy regardless of whether it will be needed.
-Kees
> +
> static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int flags)
> {
> struct linux_binprm *bprm;
> @@ -1975,6 +2005,10 @@ static int do_execveat_common(int fd, struct filename *filename,
> goto out_ret;
> }
>
> + retval = bprm_add_fixup_comm(bprm, argv);
> + if (retval != 0)
> + goto out_free;
> +
> retval = count(argv, MAX_ARG_STRINGS);
> if (retval == 0)
> pr_warn_once("process '%s' launched '%s' with NULL argv: empty string added\n",
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index e6c00e860951..0cd1f2d0e8c6 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -55,6 +55,7 @@ struct linux_binprm {
> of the time same as filename, but could be
> different for binfmt_{misc,script} */
> const char *fdpath; /* generated filename for execveat */
> + const char *argv0; /* argv0 from execveat */
> unsigned interp_flags;
> int execfd; /* File descriptor of the executable */
> unsigned long loader, exec;
--
Kees Cook
Powered by blists - more mailing lists