Bug: retry of clone() on Alpha can result in zeroed process thread pointer

Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Michael Cree]

I am seeing a bug in clone() on the Alpha architecture.  Reported to
Debian as https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=755397

The test suite of glibc sometimes fails in the nptl/tst-eintr3 test
with a segmentation fault.  I have tracked it down to the thread
pointer returned by the rduniq PALcall is occasionally zero when
it should point to the TLS.  I have only ever seen this occur when
running a SMP kernel.

Running strace on nptl/tst-eintr3 reveals that the clone() syscall
is retried by the kernel if an ERESTARTNOINTR error occurs.  At
$syscall_error in arch/alpha/kernel/entry.S the kernel handles the
error and in doing that it writes to 72(sp) which is where the value
of the a3 CPU register on entry to the kernel is stored.  Then the
kernel retries the clone() function.  But the alpha specific code
for copy_thread() in arch/alpha/kernel/process.c does not use the
passed a3 cpu register (the argument tls), instead it goes to the
saved stack to get the value of the a3 register, which on the
second call to clone() has been modified to no longer be the value
of the a3 cpu register on entry to the kernel.  And a latent bomb
is laid for userspace in the form of an incorrect process unique
value (which is the thread pointer) in the PCB.

Am I correct in my analysis and, if so, can we get a fix for this
please.

Cheers
Michael.

Re: Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Richard Henderson]

On 07/22/2014 10:52 PM, Michael Cree wrote:
> Running strace on nptl/tst-eintr3 reveals that the clone() syscall
> is retried by the kernel if an ERESTARTNOINTR error occurs.  At
> $syscall_error in arch/alpha/kernel/entry.S the kernel handles the
> error and in doing that it writes to 72(sp) which is where the value
> of the a3 CPU register on entry to the kernel is stored.  Then the
> kernel retries the clone() function.  But the alpha specific code
> for copy_thread() in arch/alpha/kernel/process.c does not use the
> passed a3 cpu register (the argument tls), instead it goes to the
> saved stack to get the value of the a3 register, which on the
> second call to clone() has been modified to no longer be the value
> of the a3 cpu register on entry to the kernel.  And a latent bomb
> is laid for userspace in the form of an incorrect process unique
> value (which is the thread pointer) in the PCB.
> 
> Am I correct in my analysis and, if so, can we get a fix for this
> please.

Well...  let me start with the assumption that we can't possibly restart unless
the syscall fails with -ERESTART*.

Before we clobber 72($sp), $syscall_error saves the old value in $19.  This is
the r19 parameter to do_work_pending, and is passed all the way down to
syscall_restart where we do restore the original value of a3 for ERESTARTNOINTR.

So if there's a path that leads to restart, but doesn't save a3 before
clobbering, I don't see it.  Do you have an strace dump that shows this?


r~

Re: Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Michael Cree]

On Thu, Jul 24, 2014 at 08:19:52AM -1000, Richard Henderson wrote:
> On 07/22/2014 10:52 PM, Michael Cree wrote:
> > Running strace on nptl/tst-eintr3 reveals that the clone() syscall
> > is retried by the kernel if an ERESTARTNOINTR error occurs.  At
> > $syscall_error in arch/alpha/kernel/entry.S the kernel handles the
> > error and in doing that it writes to 72(sp) which is where the value
> > of the a3 CPU register on entry to the kernel is stored.  Then the
> > kernel retries the clone() function.  But the alpha specific code
> > for copy_thread() in arch/alpha/kernel/process.c does not use the
> > passed a3 cpu register (the argument tls), instead it goes to the
> > saved stack to get the value of the a3 register, which on the
> > second call to clone() has been modified to no longer be the value
> > of the a3 cpu register on entry to the kernel.  And a latent bomb
> > is laid for userspace in the form of an incorrect process unique
> > value (which is the thread pointer) in the PCB.
> > 
> > Am I correct in my analysis and, if so, can we get a fix for this
> > please.
> 
> Well...  let me start with the assumption that we can't possibly restart unless
> the syscall fails with -ERESTART*.
> 
> Before we clobber 72($sp), $syscall_error saves the old value in $19.  This is
> the r19 parameter to do_work_pending, and is passed all the way down to
> syscall_restart where we do restore the original value of a3 for ERESTARTNOINTR.
> 
> So if there's a path that leads to restart, but doesn't save a3 before
> clobbering, I don't see it.  Do you have an strace dump that shows this?

Yes.  This is an example of a run of nptl/tst-eintr3 that fails after
cutting off quite a bit of stuff at the start to get to the relevant
section:

clone(child_stack=0x2000121eae0, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0x2000121f2c0, tls=0x2000121f8e0, child_tidptr=0x2000121f2c0) = ? ERESTARTNOINTR (To be restarted)
--- SIGUSR1 {si_signo=SIGUSR1, si_code=SI_TKILL, si_pid=20086, si_uid=1000} ---
write(1, ".", 1.)                        = 1
sigreturn() (mask [])                   = -1 ERRNO_312 (Unknown error 312)
clone(child_stack=0x2000121eae0, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTID, parent_tidptr=0x2000121f2c0, tls=0, child_tidptr=0x2000121f2c0) = 20089
+++ killed by SIGSEGV +++


Note that the retry of clone() has zero for the tls argument.

Examining the resultant core dump reveals that tst-eintr3 segfaulted
when trying to access a thread local variable and that register v0,
used in calculating the TLS location and set up by the rduniq PALcall,
is zero.

Cheers
Michael.

Re: Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Richard Henderson]

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

I don't think this will help, but it's worth a test, prior to totally
re-writing entry.S.  There's a chance of confusion of the value of regs->r0 if
there is in fact no error.


r~


[-- Attachment #2: z --]
[-- Type: text/plain, Size: 2145 bytes --]

diff --git a/arch/alpha/kernel/signal.c b/arch/alpha/kernel/signal.c
index 6cec288..5b855c8 100644
--- a/arch/alpha/kernel/signal.c
+++ b/arch/alpha/kernel/signal.c
@@ -452,30 +452,6 @@ handle_signal(struct ksignal *ksig, struct pt_regs *regs)
 	signal_setup_done(ret, ksig, 0);
 }
 
-static inline void
-syscall_restart(unsigned long r0, unsigned long r19,
-		struct pt_regs *regs, struct k_sigaction *ka)
-{
-	switch (regs->r0) {
-	case ERESTARTSYS:
-		if (!(ka->sa.sa_flags & SA_RESTART)) {
-		case ERESTARTNOHAND:
-			regs->r0 = EINTR;
-			break;
-		}
-		/* fallthrough */
-	case ERESTARTNOINTR:
-		regs->r0 = r0;	/* reset v0 and a3 and replay syscall */
-		regs->r19 = r19;
-		regs->pc -= 4;
-		break;
-	case ERESTART_RESTARTBLOCK:
-		regs->r0 = EINTR;
-		break;
-	}
-}
-
-
 /*
  * Note that 'init' is a special process: it doesn't get signals it doesn't
  * want to handle. Thus you cannot kill init even with a SIGKILL even by
@@ -499,13 +475,35 @@ do_signal(struct pt_regs *regs, unsigned long r0, unsigned long r19)
 	if (get_signal(&ksig)) {
 		/* ... so re-check the single stepping. */
 		single_stepping |= ptrace_cancel_bpt(current);
+
+		/* Are we coming from a syscall error?  */
+		if (r0 && regs->r19) {
+			switch (regs->r0) {
+			case ERESTARTSYS:
+				if (!(ksig.ka.sa.sa_flags & SA_RESTART)) {
+				case ERESTARTNOHAND:
+				case ERESTART_RESTARTBLOCK:
+					regs->r0 = EINTR;
+					regs->r19 = 1;
+					break;
+				}
+				/* fallthrough */
+			case ERESTARTNOINTR:
+				/* Reset v0 and a3 for syscall replay.  */
+				regs->r0 = r0;
+				regs->r19 = r19;
+				regs->pc -= 4;
+				break;
+			}
+		}
+
 		/* Whee!  Actually deliver the signal.  */
-		if (r0)
-			syscall_restart(r0, r19, regs, &ksig.ka);
 		handle_signal(&ksig, regs);
 	} else {
 		single_stepping |= ptrace_cancel_bpt(current);
-		if (r0) {
+
+		/* Are we coming from a syscall error?  */
+		if (r0 && regs->r19) {
 			switch (regs->r0) {
 			case ERESTARTNOHAND:
 			case ERESTARTSYS:
@@ -522,6 +520,7 @@ do_signal(struct pt_regs *regs, unsigned long r0, unsigned long r19)
 				break;
 			}
 		}
+
 		restore_saved_sigmask();
 	}
 	if (single_stepping)

Re: Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Michael Cree]

On Tue, Jul 29, 2014 at 12:23:45PM -1000, Richard Henderson wrote:
> I don't think this will help, but it's worth a test, prior to totally
> re-writing entry.S.  There's a chance of confusion of the value of regs->r0 if
> there is in fact no error.

No, it did not help.  The glibc nptl/tst-eintr3 test still occassionally
sefgaults and that is occurring because rduniq returned a NULL pointer.

Cheers
Michael.

Re: Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Richard Henderson]

On 07/29/2014 06:51 PM, Michael Cree wrote:
> On Tue, Jul 29, 2014 at 12:23:45PM -1000, Richard Henderson wrote:
>> I don't think this will help, but it's worth a test, prior to totally
>> re-writing entry.S.  There's a chance of confusion of the value of regs->r0 if
>> there is in fact no error.
> 
> No, it did not help.  The glibc nptl/tst-eintr3 test still occassionally
> sefgaults and that is occurring because rduniq returned a NULL pointer.

Another try.


r~



diff --git a/arch/alpha/kernel/signal.c b/arch/alpha/kernel/signal.c
index 6cec288..2d6c0e4 100644
--- a/arch/alpha/kernel/signal.c
+++ b/arch/alpha/kernel/signal.c
@@ -518,6 +518,7 @@ do_signal
                        case ERESTART_RESTARTBLOCK:
                                /* Set v0 to the restart_syscall and replay */
                                regs->r0 = __NR_restart_syscall;
+                               regs->r19 = r19;
                                regs->pc -= 4;
                                break;
                        }

Re: Bug: retry of clone() on Alpha can result in zeroed process thread pointer

[Richard Henderson]

Ok, I think I've actually found it this time.  It's here:

> 281		childregs->r20 = 1;	/* OSF/1 has some strange fork() semantics.  */
> 282		regs->r20 = 0;

We need to delay this r20 silliness until after restarts or something.  Or just
kill it -- it's not like glibc uses this return value.


r~