[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87wmixw1h6.fsf@email.froward.int.ebiederm.org>
Date: Fri, 27 Sep 2024 10:38:45 -0500
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Tycho Andersen <tycho@...ho.pizza>
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
Tycho Andersen <tycho@...ho.pizza> writes:
> 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?
Not a userspace visible issue at all.
With TASK_COMM_LEN <= DNAME_INLINE_LEN we could just use a memcpy of
TASK_COMM_LEN bytes, and everything will be safe. (But we aren't
guaranteed a terminating '\0').
If you look at d_move and copy_name in dcache.c you can see that
there are cases where a rename of the dentry that happens as we
are reading it to fill task->comm a terminating '\0' might be
missed.
strscpy_pad relies on either finding a final '\0' after which
is adds more '\0's or on finding the end of the source buffer.
strscpy_pad will guarantee that there is a final '\0' in task->comm.
There might be some race in reading dentry->d_name, but I don't think we
much care.
Eric
Powered by blists - more mailing lists