[PATCH] x86_64: another fix for canonical RIPs during signal handling

[PATCH] x86_64: another fix for canonical RIPs during signal handling

[Willy Tarreau]

I've been reported by the PaX Team that the following fix left a
small hole :

[PATCH] Always check that RIPs are canonical during signal handling

+	if (regs->rip >= TASK_SIZE && regs->rip < VSYSCALL_START) {
+		regs->rip = 0;
+		return -EFAULT;
+	}

...

+	if (regs->rip >= TASK_SIZE) {
+		if (sig == SIGSEGV)
+			ka->sa.sa_handler = SIG_DFL;
+		regs->rip = 0;
+	}

"the wrong part is regs->rip=0, i guess the intention was to cause a
 SIGSEGV upon returning to userland, but 0 is a valid userland address,
 an application may very well have something mapped there. the correct
 value would be ~0UL as it's guaranteed to fault on linux."

This explanation makes sense, so here's the patch. Andi, would you please
review and confirm ? Thanks in advance.

      Acked-By: Pax Team
      Signed-off-by: Willy Tarreau <w@1wt.eu>

---

 arch/x86_64/kernel/signal.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

a408e3e80854d04915969fe3ae05152bedf99c79
diff --git a/arch/x86_64/kernel/signal.c b/arch/x86_64/kernel/signal.c
index 8bc844f..f3eff11 100644
--- a/arch/x86_64/kernel/signal.c
+++ b/arch/x86_64/kernel/signal.c
@@ -144,7 +144,7 @@ #define COPY(x)		err |= __get_user(regs-
 	COPY(rdx); COPY(rcx); 
 	COPY(rip);
 	if (regs->rip >= TASK_SIZE && regs->rip < VSYSCALL_START) { 
-		regs->rip = 0;
+		regs->rip = ~0UL; /* force the application to fault */
 		return -EFAULT;
 	}
 	COPY(r8);
@@ -361,7 +361,7 @@ #endif
 	if (regs->rip >= TASK_SIZE) { 
 		if (sig == SIGSEGV)
 			ka->sa.sa_handler = SIG_DFL;
-		regs->rip = 0;
+		regs->rip = ~0UL; /* force the application to fault */
 	}
 	regs->cs = __USER_CS;
 	regs->ss = __USER_DS; 
-- 
1.3.3

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Andi Kleen]

On Thursday 22 June 2006 23:06, Willy Tarreau wrote:
> 
> I've been reported by the PaX Team that the following fix left a
> small hole :
> 
> [PATCH] Always check that RIPs are canonical during signal handling
> 
> +	if (regs->rip >= TASK_SIZE && regs->rip < VSYSCALL_START) {
> +		regs->rip = 0;
> +		return -EFAULT;
> +	}
> 
> ...
> 
> +	if (regs->rip >= TASK_SIZE) {
> +		if (sig == SIGSEGV)
> +			ka->sa.sa_handler = SIG_DFL;
> +		regs->rip = 0;
> +	}
> 
> "the wrong part is regs->rip=0, i guess the intention was to cause a
>  SIGSEGV upon returning to userland, but 0 is a valid userland address,
>  an application may very well have something mapped there. the correct
>  value would be ~0UL as it's guaranteed to fault on linux."
> 
> This explanation makes sense, so here's the patch. Andi, would you please
> review and confirm ? Thanks in advance.

I don't think it's a real problem.

The patch is not wrong, but also doesn't fix something that needs
to be fixed.

-Andi

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Willy Tarreau]

On Thu, Jun 22, 2006 at 11:26:22PM +0200, Andi Kleen wrote:
> On Thursday 22 June 2006 23:06, Willy Tarreau wrote:
> > 
> > I've been reported by the PaX Team that the following fix left a
> > small hole :
> > 
> > [PATCH] Always check that RIPs are canonical during signal handling
> > 
> > +	if (regs->rip >= TASK_SIZE && regs->rip < VSYSCALL_START) {
> > +		regs->rip = 0;
> > +		return -EFAULT;
> > +	}
> > 
> > ...
> > 
> > +	if (regs->rip >= TASK_SIZE) {
> > +		if (sig == SIGSEGV)
> > +			ka->sa.sa_handler = SIG_DFL;
> > +		regs->rip = 0;
> > +	}
> > 
> > "the wrong part is regs->rip=0, i guess the intention was to cause a
> >  SIGSEGV upon returning to userland, but 0 is a valid userland address,
> >  an application may very well have something mapped there. the correct
> >  value would be ~0UL as it's guaranteed to fault on linux."
> > 
> > This explanation makes sense, so here's the patch. Andi, would you please
> > review and confirm ? Thanks in advance.
> 
> I don't think it's a real problem.
> 
> The patch is not wrong, but also doesn't fix something that needs
> to be fixed.

What I understand from this is if code is mapped at 0 (eg by mmap(PROT_EXEC)),
it would get executed instead of the program being killed. Although I don't
see how this could be exploited to gain any privileges, I wonder if it can
cause a process to loop indefinitely instead of being killed or nasty things
like this. May be this is a stupid analysis from me, so I hope that PaX Team
will have more precise info.

> -Andi

Regards,
Willy

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Andi Kleen]

> What I understand from this is if code is mapped at 0 (eg by mmap(PROT_EXEC)),
> it would get executed instead of the program being killed. Although I don't
> see how this could be exploited to gain any privileges, I wonder if it can
> cause a process to loop indefinitely instead of being killed or nasty things
> like this. May be this is a stupid analysis from me, so I hope that PaX Team
> will have more precise info.

When you can inject a non canonical RIP into the stack frame you can likely 
also inject other RIPs.  So the whole thing is just a funny way for a program to 
jump in its address space.  0 is as good as any other address for this.

The whole point of the check is just to protect the kernel/CPU against this.
What happens to the user space program itself is no concern because it is the program's
own doing.

-Andi

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Andi Kleen]

> 
> that's not true. if the application expects to crash due to a bad
> signal handler then rip=0 may or may not achieve that, depending on
> what mapping exists at that address - this is inconsistent behaviour
> (from userland's point of view) created by the kernel itself, hence
> this is a kernel bug and should be fixed.

If it "wants" to crash it can just jump to 0 (or whatever unmapped address
it has) by itself. No need to involve the kernel here.

The only point of the patch was to not make the kernel/CPU crash due 
to CPU bugs triggered by applications. But we really
don't care what happens to the application when it corrupts its stack frame.

-Andi

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Willy Tarreau]

On Fri, Jun 23, 2006 at 03:17:42PM +0200, pageexec@freemail.hu wrote:
> > > that's not true. if the application expects to crash due to a bad
> > > signal handler then rip=0 may or may not achieve that, depending on
> > > what mapping exists at that address - this is inconsistent behaviour
> > > (from userland's point of view) created by the kernel itself, hence
> > > this is a kernel bug and should be fixed.
> > 
> > If it "wants" to crash it can just jump to 0 (or whatever unmapped address
> > it has) by itself.
> 
> i very carefully didn't say 'want' above, instead i said 'expect'. the
> current code is breaking the expectation that invalid memory dereferences
> will cause a SIGSEGV because the rip=0 code tries to outsmart userland by
> finding such an invalid address - except 0 is not at all guaranteed to be
> invalid. don't think of only 'normal' applications where this assumption
> is mostly true, think of everything that userland may want to do and having
> a mapping at 0 is within the game rules.
> 
> > No need to involve the kernel here.
> 
> but the current code does exactly that. it assumes that it will crash
> the application by jumping to 0 which may or may not be true. the kernel
> has no business making such assumptions, if it wants to trigger an event
> in userland, it had better make sure it'll actually happen, regardless
> what userland may have done.
> 
> > The only point of the patch was to not make the kernel/CPU crash due 
> > to CPU bugs triggered by applications.
> 
> and was it also the purpose to make the application behave differently
> depending on what it has mapped at 0? i doubt so. also, what does 2.6
> do to avoid this? it doesn't have this rip=0 code (yet?).
> 
> > But we really
> > don't care what happens to the application when it corrupts its stack frame.
> 
> then why do you (try to) crash it? apparently you do care about it ;-).
> 
> in particular, the bad signal handler installed by userland would cause a
> SIGSEGV (modulo the CPU bug?), so what the original rip=0 patch wanted to
> do is trigger this SIGSEGV while not tripping on the CPU bug. it achieved
> the second goal but not the first one, that's all i'm trying to explain.

If I understand it well, an application which maps address 0 has no way to
be notified that the kernel detected a corrupted stack pointer. I agree
that if the proposed patch avoids to make this undesired distinction between
apps that map addr 0 and those which don't, it would be better to merge it.
Andi, you said there was nothing wrong with it, do you accept that it gets
merged ?

Regards,
Willy

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Andi Kleen]

> If I understand it well, an application which maps address 0 has no way to
> be notified that the kernel detected a corrupted stack pointer. 

It will just not crash again after the application tried to deliberately
crash the kernel.

> I agree 
> that if the proposed patch avoids to make this undesired distinction between
> apps that map addr 0 and those which don't, it would be better to merge it.
> Andi, you said there was nothing wrong with it, do you accept that it gets
> merged ?

As I said, it's not wrong, just not necessary.

-Andi

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Willy Tarreau]

On Fri, Jun 23, 2006 at 03:46:21PM +0200, Andi Kleen wrote:
> 
> > If I understand it well, an application which maps address 0 has no way to
> > be notified that the kernel detected a corrupted stack pointer. 
> 
> It will just not crash again after the application tried to deliberately
> crash the kernel.

"deliberately" is a bit exagerated here. Failed stack overflows,
hardware memory corruption and various bugs that happen to most
application developpers at early coding stage are not what can be
called "deliberate".

Also, I don't know if memory leak detectors rely on getting a SEGV,
but this patch would make them useless on apps which map addr 0.

> > I agree 
> > that if the proposed patch avoids to make this undesired distinction between
> > apps that map addr 0 and those which don't, it would be better to merge it.
> > Andi, you said there was nothing wrong with it, do you accept that it gets
> > merged ?
> 
> As I said, it's not wrong, just not necessary.

I understand your point, but I think that covering most situations the
same way helps reducing exceptions, and helps troubleshooting.

> -Andi

Regards,
Willy

Re: [PATCH] x86_64: another fix for canonical RIPs during signal handling

[Alan Cox]

Ar Gwe, 2006-06-23 am 15:32 +0200, ysgrifennodd Willy Tarreau:
> If I understand it well, an application which maps address 0 has no way to
> be notified that the kernel detected a corrupted stack pointer. 

A debugger can deal with this corner case if it ever needs to be careful
mapping and remapping of the page. Very few apps map data at the zero
page and those that do usually map a single zero page which probably
isn't PROT_EXEC anyway.

(The usual case is to speed up pointer chain walking by knowing that
NULL->NULL->... is always NULL)

Alan