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

Linus Torvalds
2024-11-26T20:12:35Z

[-- Attachment #1: Type: text/plain, Size: 238 bytes --]

On Tue, 26 Nov 2024 at 12:11, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So something like the attached. But it's *ENTIRELY* untested. [..]

It was also entirely un-attached. Here's the actual attachment.

Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 2623 bytes --]

fs/exec.c | 27 +++++++++++++++++++++++----
include/linux/binfmts.h | 1 +
2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index da51ca70489a..850421445ccb 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1194,12 +1194,22 @@ static int unshare_sighand(struct task_struct *me)
* so that a new one can be started
*/

+/*
+ * No locking, instead this just makes sure that comm[] is always
+ * NUL-terminated. Note that 'strscpy_pad()' does not guarantee
+ * that at least right now - it may do the copy without a NUL and
+ * then overwrite the last character.
+ *
+ * One of the few cases where 'strncpy()' does the right thing.
+ *
+ * If you do concurrent set_task_comm() calls, the name may be
+ * a bit random, but it's going to be NUL-terminated.
+ */
void __set_task_comm(struct task_struct *tsk, const char *buf, bool exec)
{
- task_lock(tsk);
trace_task_rename(tsk, buf);
- strscpy_pad(tsk->comm, buf, sizeof(tsk->comm));
- task_unlock(tsk);
+ tsk->comm[sizeof(tsk->comm)-1] = 0;
+ strncpy(tsk->comm, buf, sizeof(tsk->comm)-1);
perf_event_comm(tsk, exec);
}

@@ -1337,7 +1347,7 @@ 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);
+ __set_task_comm(me, bprm->comm, true);

/* An exec changes our domain. We are no longer part of the thread
group */
@@ -1510,6 +1520,11 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl

if (fd == AT_FDCWD || filename->name[0] == '/') {
bprm->filename = filename->name;
+ /*
+ * FIXME(?): this could take advantage of 'filename->len'
+ * to do better than kbasename()
+ */
+ strscpy(bprm->comm, kbasename(filename->name));
} else {
if (filename->name[0] == '\0')
bprm->fdpath = kasprintf(GFP_KERNEL, "/dev/fd/%d", fd);
@@ -1532,6 +1547,10 @@ static struct linux_binprm *alloc_bprm(int fd, struct filename *filename, int fl
bprm->interp_flags |= BINPRM_FLAGS_PATH_INACCESSIBLE;

bprm->filename = bprm->fdpath;
+
+ rcu_read_lock();
+ strscpy(bprm->comm, smp_load_acquire(&file->f_path.dentry->d_name.name));
+ rcu_read_unlock();
}
bprm->interp = bprm->filename;

diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index e6c00e860951..3c8bc84ca9e4 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -61,6 +61,7 @@ struct linux_binprm {

struct rlimit rlim_stack; /* Saved RLIMIT_STACK used during exec. */

+ char comm[TASK_COMM_LEN];
char buf[BINPRM_BUF_SIZE];
} __randomize_layout;