[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZvbHlChEmj35+jHF@tycho.pizza>
Date: Fri, 27 Sep 2024 08:56:20 -0600
From: Tycho Andersen <tycho@...ho.pizza>
To: "Eric W. Biederman" <ebiederm@...ssion.com>
Cc: Aleksa Sarai <cyphar@...har.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
Kees Cook <kees@...nel.org>, 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>
Subject: Re: [RFC] exec: add a flag for "reasonable" execveat() comm
On Fri, Sep 27, 2024 at 09:43:22AM -0500, Eric W. Biederman wrote:
> Tycho Andersen <tycho@...ho.pizza> writes:
>
> > On Wed, Sep 25, 2024 at 09:09:18PM -0500, Eric W. Biederman wrote:
> >> Tycho Andersen <tycho@...ho.pizza> writes:
> >>
> >> > Yep, I did this for the test above, and it worked fine:
> >> >
> >> > if (bprm->fdpath) {
> >> > /*
> >> > * 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.
> >> > */
> >> > struct path root;
> >> > char *path, buf[1024];
> >> >
> >> > get_fs_root(current->fs, &root);
> >> > path = __d_path(&bprm->file->f_path, &root, buf, sizeof(buf));
> >> >
> >> > __set_task_comm(me, kbasename(path), true);
> >> > } else {
> >> > __set_task_comm(me, kbasename(bprm->filename), true);
> >> > }
> >> >
> >> > obviously we don't want a stack allocated buffer, but triggering on
> >> > ->fdpath != NULL seems like the right thing, so we won't need a flag
> >> > either.
> >> >
> >> > The question is: argv[0] or __d_path()?
> >>
> >> You know. I think we can just do:
> >>
> >> BUILD_BUG_ON(DNAME_INLINE_LEN >= TASK_COMM_LEN);
> >> __set_task_comm(me, bprm->file->f_path.dentry->d_name.name, true);
> >>
> >> Barring cache misses that should be faster and more reliable than what
> >> we currently have and produce the same output in all of the cases we
> >> like, and produce better output in all of the cases that are a problem
> >> today.
> >>
> >> Does anyone see any problem with that?
> >
> > Nice, this works great. We need to drop the BUILD_BUG_ON() since it is
> > violated in today's tree, but I think this is safe to do anyway since
> > __set_task_comm() does strscpy_pad(tsk->comm, buf, sizeof(tsk->comm)).
>
> Doh. I simply put the conditional in the wrong order. That should have
> been:
> BUILD_BUG_ON(TASK_COMM_LEN > DNAME_INLINE_LEN);
>
> Sorry I was thinking of the invariant that needs to be preserved rather
> than the bug that happens.
Thanks, I will include that. Just for my own education: this is still
*safe* to do, because of _pad, it's just that it is a userspace
visible break if TASK_COMM_LEN > DNAME_INLINE_LEN is ever true?
Tycho
Powered by blists - more mailing lists