[PATCH] exec: remove redundant save asides of old pid/vpid

[PATCH] exec: remove redundant save asides of old pid/vpid

[Nir Lichtman]

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(-)

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;
 }
-- 
2.39.5

Re: [PATCH] exec: remove redundant save asides of old pid/vpid

[Kees Cook]

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;
> }

-- 
Kees Cook

Re: [PATCH] exec: remove redundant save asides of old pid/vpid

[Nir Lichtman]

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