| Message ID | 20250201083127.GA1185473@lichtman.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | exec: remove redundant save asides of old pid/vpid | expand |
On February 1, 2025 12:31:27 AM PST, Nir Lichtman <nir@lichtman.org> wrote: >Problem: Old pid and vpid are redundantly saved aside before starting to >parse the binary, with the comment claiming that it is required since >load_binary changes it, though from inspection in the source, >load_binary does not change the pid and this wouldn't make sense since >execve does not create any new process, quote from man page of execve: >"there is no new process; many attributes of the calling process remain >unchanged (in particular, its PID)." See commit bb188d7e64de ("ptrace: make former thread ID available via PTRACE_GETEVENTMSG after PTRACE_EVENT_EXEC stop") This is for making sense of a concurrent exec made by a multi threaded process. Specifically see de_thread(), where the pid *can* change: /* * At this point all other threads have exited, all we have to * do is to wait for the thread group leader to become inactive, * and to assume its PID: */ The described problem in the commit hasn't changed, so this code needs to stay as-is. Or perhaps the comment could be improved? -Kees > >Solution: Remove the saving aside of both and later on use them directly >from the current object, instead of via the saved aside objects. > >Signed-off-by: Nir Lichtman <nir@lichtman.org> >--- > >Side-note: Tested this solution with a defconfig x86_64 and an initramfs >with Busybox and confirmed to work fine. > > fs/exec.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > >diff --git a/fs/exec.c b/fs/exec.c >index 506cd411f4ac..6bb0a7b15f7e 100644 >--- a/fs/exec.c >+++ b/fs/exec.c >@@ -1789,15 +1789,8 @@ static int search_binary_handler(struct linux_binprm *bprm) > /* binfmt handlers will call back into begin_new_exec() on success. */ > static int exec_binprm(struct linux_binprm *bprm) > { >- pid_t old_pid, old_vpid; > int ret, depth; > >- /* Need to fetch pid before load_binary changes it */ >- old_pid = current->pid; >- rcu_read_lock(); >- old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent)); >- rcu_read_unlock(); >- > /* This allows 4 levels of binfmt rewrites before failing hard. */ > for (depth = 0;; depth++) { > struct file *exec; >@@ -1826,8 +1819,9 @@ static int exec_binprm(struct linux_binprm *bprm) > } > > audit_bprm(bprm); >- trace_sched_process_exec(current, old_pid, bprm); >- ptrace_event(PTRACE_EVENT_EXEC, old_vpid); >+ trace_sched_process_exec(current, current->pid, bprm); >+ ptrace_event(PTRACE_EVENT_EXEC, >+ task_pid_nr_ns(current, task_active_pid_ns(current->parent))); > proc_exec_connector(current); > return 0; > }
On Sat, Feb 01, 2025 at 01:40:00AM -0800, Kees Cook wrote: > > > On February 1, 2025 12:31:27 AM PST, Nir Lichtman <nir@lichtman.org> wrote: > >Problem: Old pid and vpid are redundantly saved aside before starting to > >parse the binary, with the comment claiming that it is required since > >load_binary changes it, though from inspection in the source, > >load_binary does not change the pid and this wouldn't make sense since > >execve does not create any new process, quote from man page of execve: > >"there is no new process; many attributes of the calling process remain > >unchanged (in particular, its PID)." > > See commit bb188d7e64de ("ptrace: make former thread ID available via PTRACE_GETEVENTMSG after PTRACE_EVENT_EXEC stop") > > This is for making sense of a concurrent exec made by a multi threaded process. Specifically see de_thread(), where the pid *can* change: > > /* > * At this point all other threads have exited, all we have to > * do is to wait for the thread group leader to become inactive, > * and to assume its PID: > */ > > The described problem in the commit hasn't changed, so this code needs to stay as-is. Or perhaps the comment could be improved? Thanks for answering, interesting, I'll take a deeper look. Nir
diff --git a/fs/exec.c b/fs/exec.c index 506cd411f4ac..6bb0a7b15f7e 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1789,15 +1789,8 @@ static int search_binary_handler(struct linux_binprm *bprm) /* binfmt handlers will call back into begin_new_exec() on success. */ static int exec_binprm(struct linux_binprm *bprm) { - pid_t old_pid, old_vpid; int ret, depth; - /* Need to fetch pid before load_binary changes it */ - old_pid = current->pid; - rcu_read_lock(); - old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent)); - rcu_read_unlock(); - /* This allows 4 levels of binfmt rewrites before failing hard. */ for (depth = 0;; depth++) { struct file *exec; @@ -1826,8 +1819,9 @@ static int exec_binprm(struct linux_binprm *bprm) } audit_bprm(bprm); - trace_sched_process_exec(current, old_pid, bprm); - ptrace_event(PTRACE_EVENT_EXEC, old_vpid); + trace_sched_process_exec(current, current->pid, bprm); + ptrace_event(PTRACE_EVENT_EXEC, + task_pid_nr_ns(current, task_active_pid_ns(current->parent))); proc_exec_connector(current); return 0; }
Problem: Old pid and vpid are redundantly saved aside before starting to parse the binary, with the comment claiming that it is required since load_binary changes it, though from inspection in the source, load_binary does not change the pid and this wouldn't make sense since execve does not create any new process, quote from man page of execve: "there is no new process; many attributes of the calling process remain unchanged (in particular, its PID)." Solution: Remove the saving aside of both and later on use them directly from the current object, instead of via the saved aside objects. Signed-off-by: Nir Lichtman <nir@lichtman.org> --- Side-note: Tested this solution with a defconfig x86_64 and an initramfs with Busybox and confirmed to work fine. fs/exec.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)