linux-kernel - Re: [PATCH] exec: fix up /proc/pid/comm in the execveat(AT_EMPTY

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241201-konglomerat-genial-c1344842c88b@brauner>
Date: Sun, 1 Dec 2024 15:17:13 +0100
From: Christian Brauner <brauner@...nel.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Kees Cook <kees@...nel.org>, Al Viro <viro@...iv.linux.org.uk>, 
	Zbigniew Jędrzejewski-Szmek <zbyszek@...waw.pl>, Tycho Andersen <tandersen@...flix.com>, 
	Aleksa Sarai <cyphar@...har.com>, Eric Biederman <ebiederm@...ssion.com>, Jan Kara <jack@...e.cz>, 
	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-hardening@...r.kernel.org
Subject: Re: [PATCH] exec: fix up /proc/pid/comm in the
 execveat(AT_EMPTY_PATH) case

On Sat, Nov 30, 2024 at 10:02:38AM -0800, Linus Torvalds wrote:
> On Sat, 30 Nov 2024 at 04:30, Christian Brauner <brauner@...nel.org> wrote:
> >
> > What does the smp_load_acquire() pair with?
> 
> I'm not sure we have them everywhere, but at least this one at dentry
> creation time.
> 
> __d_alloc():
>         /* Make sure we always see the terminating NUL character */
>         smp_store_release(&dentry->d_name.name, dname); /* ^^^ */
> 
> so even at rename time, when we swap the d_name.name pointers
> (*without* using a store-release at that time), both of the dentry
> names had memory orderings before.
> 
> That said, looking at swap_name() at the non-"swap just the pointers"
> case, there we do just "memcpy()" the name, and it would probably be
> good to update the target d_name.name with a smp_store_release.
> 
> In practice, none of this ever matters. Anybody who uses the dentry
> name without locking either doesn't care enough (like comm[]) or will
> use the sequence number thing to serialize at a much higher level. So
> the smp_load_acquire() could probably be a READ_ONCE(), and nobody
> would ever see the difference.

Right now it's confusing. So no matter if we do READ_ONCE() or
smp_load_acquire() there'd please be a comment explaing why so we don't
pointlessly leave everyone wondering about that barrier.

/*
 * Hold rcu lock to keep the name from being freed behind our back.
 * Use cquire semantics to make sure the terminating NUL from
 * __d_alloc() is seen.
 *
 * Note, we're deliberately sloppy here. We don't need to care about
 * detecting a concurrent rename and just want a sensible name.
 */
rcu_read_lock();
__set_task_comm(me, smp_load_acquire(&file_dentry(bprm->file)->d_name.name), true);
rcu_read_unlock();

or something better.