linux-kernel - Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

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: <874j3qnrfc.fsf@email.froward.int.ebiederm.org>
Date: Fri, 29 Nov 2024 06:41:11 -0600
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Al Viro <viro@...iv.linux.org.uk>,  Kees Cook <kees@...nel.org>,
  linux-kernel@...r.kernel.org,  Christophe JAILLET
 <christophe.jaillet@...adoo.fr>,  Nir Lichtman <nir@...htman.org>,  Tycho
 Andersen <tandersen@...flix.com>,  Vegard Nossum
 <vegard.nossum@...cle.com>
Subject: Re: [GIT PULL] execve updates for v6.13-rc1 (take 2)

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

> On Thu, 28 Nov 2024 at 19:34, Al Viro <viro@...iv.linux.org.uk> wrote:
>>
>> Just one thing - IMO we want to use the relative pathname when it's
>> not empty.  Even in execveat()
>
> Oh, absolutely agreed.
>
> Good catch, because yes, I messed that part up in my suggested patch at
>
>    https://lore.kernel.org/all/CAHk-=wjF_09Z6vu7f8UAbQVDDoHnd-j391YpUxmBPLD=SKbKtQ@mail.gmail.com/
>
> which did this dentry name thing for anything that used a base fd, but
> yes, as you say, it should only do it when there is no name at all.
>
> So instead of basing it (incorrectly) on that existing
>
>         if (fd == AT_FDCWD || filename->name[0] == '/') {
>
> test, the logic should probably look something like
>
>         if (!filename->name[0]) {
>                 rcu_read_lock();
>                 strscpy(bprm->comm,
> smp_load_acquire(&file->f_path.dentry->d_name.name));
>                 rcu_read_unlock();
>         } else
>                 strscpy(bprm->comm, kbasename(filename->name));
>
> and it probably wouldn't be a bad idea to separate this out to be a
> helper function that just does this one thing.

Yes.

It probably makes sense to change __set_task_comm
to something simple like:

void __set_task_comm(struct task_struct *tsk, const char *buf[TASK_COMM_LEN], bool exec)
{
	trace_task_rename(tsk, buf);
        memcpy(tsk->comm, buf, TASK_COMM_LEN);
	perf_event_comm(tsk, exec);
}

There are only 6 callers of set_task_comm and __set_task_comm and they
are straight forward to verify that they pass in a TASK_COMM_LEN bytes
that are already initialized.  With the bytes already properly
initialized just copying them looks like it is as safe as anything else
when it comes to races.

The callers need to be tweaked a little to meet that condition
something like:

diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c
index a38f36b68060..5d0928f37471 100644
--- a/io_uring/io-wq.c
+++ b/io_uring/io-wq.c
@@ -634,7 +634,7 @@ static int io_wq_worker(void *data)
	struct io_wq_acct *acct = io_wq_get_acct(worker);
	struct io_wq *wq = worker->wq;
	bool exit_mask = false, last_timeout = false;
-	char buf[TASK_COMM_LEN];
+	char buf[TASK_COMM_LEN] = {};
 
	set_mask_bits(&worker->flags, 0,
		      BIT(IO_WORKER_F_UP) | BIT(IO_WORKER_F_RUNNING));
diff --git a/io_uring/sqpoll.c b/io_uring/sqpoll.c
index 6df5e649c413..701390bbb303 100644
--- a/io_uring/sqpoll.c
+++ b/io_uring/sqpoll.c
@@ -264,7 +264,7 @@ static int io_sq_thread(void *data)
	struct io_ring_ctx *ctx;
	struct rusage start;
	unsigned long timeout = 0;
-	char buf[TASK_COMM_LEN];
+	char buf[TASK_COMM_LEN] = {};
	DEFINE_WAIT(wait);
 
	/* offload context creation failed, just exit */
diff --git a/kernel/kthread.c b/kernel/kthread.c
index a5ac612b1609..2b126835d728 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -738,10 +738,11 @@ EXPORT_SYMBOL(kthread_stop_put);
 
 int kthreadd(void *unused)
 {
+	static const char comm[TASK_COMM_LEN] = "kthreadd";
	struct task_struct *tsk = current;
 
	/* Setup a clean context for our children to inherit. */
-	set_task_comm(tsk, "kthreadd");
+	set_task_comm(tsk, comm);
	ignore_signals(tsk);
	set_cpus_allowed_ptr(tsk, housekeeping_cpumask(HK_TYPE_KTHREAD));
	set_mems_allowed(node_states[N_MEMORY]);


Eric