diff mbox series

[v2] sparc: make copy_thread honor pid namespaces

Message ID 20210219225030.GA23520@altlinux.org
State New
Headers show
Series [v2] sparc: make copy_thread honor pid namespaces | expand

Commit Message

Dmitry V. Levin Feb. 19, 2021, 10:50 p.m. UTC
On sparc, fork and clone syscalls have an unusual semantics of
returning the pid of the parent process to the child process.

Apparently, the implementation did not honor pid namespaces at all,
so the child used to get the pid of its parent in the init namespace.

Fortunately, most users of these syscalls are not affected by this bug
because they use another register to distinguish the parent process
from its child, and the pid of the parent process is often discarded.

Reproducer:

  $ gcc -Wall -O2 -xc - <<'EOF'
  #define _GNU_SOURCE
  #include <err.h>
  #include <errno.h>
  #include <sched.h>
  #include <stdio.h>
  #include <stdlib.h>
  #include <sys/wait.h>
  #include <unistd.h>
  #include <asm/unistd.h>
  #include <linux/sched.h>
  static void test_fork(void)
  {
  	int pid = syscall(__NR_fork);
  	if (pid < 0)
  		err(1, "fork");
  	fprintf(stderr, "current: %d, parent: %d, fork returned: %d\n",
  		getpid(), getppid(), pid);
  	int status;
  	if (wait(&status) < 0) {
  		if (errno == ECHILD)
  			_exit(0);
  		err(1, "wait");
  	}
  	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
  		errx(1, "wait: %#x", status);
  }
  int main(void)
  {
  	test_fork();
  	if (unshare(CLONE_NEWPID | CLONE_NEWUSER) < 0)
  		err(1, "unshare");
  	test_fork();
  	return 0;
  }
  EOF
  $ sh -c ./a.out
  current: 10001, parent: 10000, fork returned: 10002
  current: 10002, parent: 10001, fork returned: 10001
  current: 10001, parent: 10000, fork returned: 10003
  current: 1, parent: 0, fork returned: 10001

This bug was found by strace test suite.

Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: stable@vger.kernel.org
Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
---

v2: Replaced task_active_pid_ns(p) with current->nsproxy->pid_ns_for_children
    as suggested by Eric.

 arch/sparc/kernel/process_32.c | 3 ++-
 arch/sparc/kernel/process_64.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Eric W. Biederman Feb. 22, 2021, 9:30 p.m. UTC | #1
"Dmitry V. Levin" <ldv@altlinux.org> writes:

> On sparc, fork and clone syscalls have an unusual semantics of
> returning the pid of the parent process to the child process.
>
> Apparently, the implementation did not honor pid namespaces at all,
> so the child used to get the pid of its parent in the init namespace.
>
> Fortunately, most users of these syscalls are not affected by this bug
> because they use another register to distinguish the parent process
> from its child, and the pid of the parent process is often discarded.
>
> Reproducer:
>
>   $ gcc -Wall -O2 -xc - <<'EOF'
>   #define _GNU_SOURCE
>   #include <err.h>
>   #include <errno.h>
>   #include <sched.h>
>   #include <stdio.h>
>   #include <stdlib.h>
>   #include <sys/wait.h>
>   #include <unistd.h>
>   #include <asm/unistd.h>
>   #include <linux/sched.h>
>   static void test_fork(void)
>   {
>   	int pid = syscall(__NR_fork);
>   	if (pid < 0)
>   		err(1, "fork");
>   	fprintf(stderr, "current: %d, parent: %d, fork returned: %d\n",
>   		getpid(), getppid(), pid);
>   	int status;
>   	if (wait(&status) < 0) {
>   		if (errno == ECHILD)
>   			_exit(0);
>   		err(1, "wait");
>   	}
>   	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
>   		errx(1, "wait: %#x", status);
>   }
>   int main(void)
>   {
>   	test_fork();
>   	if (unshare(CLONE_NEWPID | CLONE_NEWUSER) < 0)
>   		err(1, "unshare");
>   	test_fork();
>   	return 0;
>   }
>   EOF
>   $ sh -c ./a.out
>   current: 10001, parent: 10000, fork returned: 10002
>   current: 10002, parent: 10001, fork returned: 10001
>   current: 10001, parent: 10000, fork returned: 10003
>   current: 1, parent: 0, fork returned: 10001
>
> This bug was found by strace test suite.
>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Dmitry V. Levin <ldv@altlinux.org>
> ---
>
> v2: Replaced task_active_pid_ns(p) with current->nsproxy->pid_ns_for_children
>     as suggested by Eric.

I can't test this either.  But from just reading through the code earlier.
Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>

>  arch/sparc/kernel/process_32.c | 3 ++-
>  arch/sparc/kernel/process_64.c | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/arch/sparc/kernel/process_32.c b/arch/sparc/kernel/process_32.c
> index a02363735915..3be653e40204 100644
> --- a/arch/sparc/kernel/process_32.c
> +++ b/arch/sparc/kernel/process_32.c
> @@ -368,7 +368,8 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
>  #endif
>  
>  	/* Set the return value for the child. */
> -	childregs->u_regs[UREG_I0] = current->pid;
> +	childregs->u_regs[UREG_I0] =
> +		task_pid_nr_ns(current, current->nsproxy->pid_ns_for_children);
>  	childregs->u_regs[UREG_I1] = 1;
>  
>  	/* Set the return value for the parent. */
> diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
> index 6f8c7822fc06..f53ef5cff46a 100644
> --- a/arch/sparc/kernel/process_64.c
> +++ b/arch/sparc/kernel/process_64.c
> @@ -629,7 +629,8 @@ int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
>  		t->utraps[0]++;
>  
>  	/* Set the return value for the child. */
> -	t->kregs->u_regs[UREG_I0] = current->pid;
> +	t->kregs->u_regs[UREG_I0] =
> +		task_pid_nr_ns(current, current->nsproxy->pid_ns_for_children);
>  	t->kregs->u_regs[UREG_I1] = 1;
>  
>  	/* Set the second return value for the parent. */
Anatoly Pugachev Feb. 23, 2021, 11:14 a.m. UTC | #2
On Sat, Feb 20, 2021 at 1:51 AM Dmitry V. Levin <ldv@altlinux.org> wrote:
>
> On sparc, fork and clone syscalls have an unusual semantics of
> returning the pid of the parent process to the child process.
>
> Apparently, the implementation did not honor pid namespaces at all,
> so the child used to get the pid of its parent in the init namespace.
>
> Fortunately, most users of these syscalls are not affected by this bug
> because they use another register to distinguish the parent process
> from its child, and the pid of the parent process is often discarded.
>
> Reproducer:

tested on my sparc64 ldom:

1. non-patched kernel (kernel 5.11.0-03615-g55f62bc87347), reproducer/test run:

$ gcc -Wall -O2 sparc-fork-namespace2.c
$ ./a.out
current: 72544, parent: 69045, fork returned: 72545
current: 72545, parent: 72544, fork returned: 72544
current: 72544, parent: 69045, fork returned: 72546
current: 1, parent: 0, fork returned: 72544

2. kernel patch applied, test run:

$ uname -a
Linux ttip 5.11.0-09279-g7c70f3a7488d-dirty #185 SMP Tue Feb 23
00:50:11 MSK 2021 sparc64 GNU/Linux

$ ./a.out
current: 939, parent: 918, fork returned: 940
current: 940, parent: 939, fork returned: 939
current: 939, parent: 918, fork returned: 941
a.out: wait: 0x9

and console/kernel logs:

[   72.204212] Kernel unaligned access at TPC[4060c8] ret_from_fork+0x1c/0x2c
[   72.204254] Unsupported unaligned load/store trap for kernel at
<00000000004060c8>.
[   72.204267]               \|/ ____ \|/
[   72.204267]               "@'/ .. \`@"
[   72.204267]               /_| \__/ |_\
[   72.204267]                  \__U_/
[   72.204284] a.out(941): Kernel does fpu/atomic unaligned load/store. [#3]
[   72.204298] CPU: 6 PID: 941 Comm: a.out Tainted: G      D     E
5.11.0-09279-g7c70f3a7488d-dirty #185
[   72.204315] TSTATE: 0000004411001600 TPC: 00000000004060c8 TNPC:
00000000004060cc Y: 000000cf    Tainted: G      D     E
[   72.204330] TPC: <ret_from_fork+0x1c/0x2c>
[   72.204340] g0: 0000000000000000 g1: ffffffffffffffff g2:
0000000000000006 g3: 0000000000000000
[   72.204349] g4: fff8000041482700 g5: fff80021a9890000 g6:
fff8000041038000 g7: 0308000108000004
[   72.204359] o0: 0000000000000000 o1: 000000001fffffff o2:
fff8000041482780 o3: 0000000000000000
[   72.204369] o4: 0000000000000001 o5: 0000000000000000 sp:
fff800004103b6a1 ret_pc: 00000000004060b0
[   72.204378] RPC: <ret_from_fork+0x4/0x2c>
[   72.204387] l0: 0308000108000000 l1: 0000000000000002 l2:
0000000000450cf4 l3: 0000000000000000
[   72.204396] l4: 0000000000000264 l5: 00000100001eb9f0 l6:
fff8000045018000 l7: 00000000006c7840
[   72.204406] i0: 0000000000efcd60 i1: 0000000000450df4 i2:
00000000000003dc i3: 00000000000003dc
[   72.204415] i4: 0000000000eebc78 i5: 0000000000000000 i6:
000007feffa56811 i7: 0000010000000ad8
[   72.204424] I7: <0x10000000ad8>
[   72.204432] Call Trace:
[   72.204439] Caller[0000010000000ad8]: 0x10000000ad8
[   72.204446] Instruction DUMP:
[   72.204449]  0aca0072
[   72.204456]  e059a008
[   72.204461]  e25ba8c7
[   72.204467] <9fc44000>
[   72.204472]  d05ba8cf
[   72.204477]  1068006d
[   72.204483]  90102000
[   72.204488]  0f0011a9
[   72.204493]  10680004
[   72.204499]


So something is actually wrong with the patch.

Thanks.
diff mbox series

Patch

diff --git a/arch/sparc/kernel/process_32.c b/arch/sparc/kernel/process_32.c
index a02363735915..3be653e40204 100644
--- a/arch/sparc/kernel/process_32.c
+++ b/arch/sparc/kernel/process_32.c
@@ -368,7 +368,8 @@  int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
 #endif
 
 	/* Set the return value for the child. */
-	childregs->u_regs[UREG_I0] = current->pid;
+	childregs->u_regs[UREG_I0] =
+		task_pid_nr_ns(current, current->nsproxy->pid_ns_for_children);
 	childregs->u_regs[UREG_I1] = 1;
 
 	/* Set the return value for the parent. */
diff --git a/arch/sparc/kernel/process_64.c b/arch/sparc/kernel/process_64.c
index 6f8c7822fc06..f53ef5cff46a 100644
--- a/arch/sparc/kernel/process_64.c
+++ b/arch/sparc/kernel/process_64.c
@@ -629,7 +629,8 @@  int copy_thread(unsigned long clone_flags, unsigned long sp, unsigned long arg,
 		t->utraps[0]++;
 
 	/* Set the return value for the child. */
-	t->kregs->u_regs[UREG_I0] = current->pid;
+	t->kregs->u_regs[UREG_I0] =
+		task_pid_nr_ns(current, current->nsproxy->pid_ns_for_children);
 	t->kregs->u_regs[UREG_I1] = 1;
 
 	/* Set the second return value for the parent. */