[PATCH] information leak in sigaltstack

[PATCH] information leak in sigaltstack

[Ulrich Drepper]

Unfortunately the stack_t data structure was defined before people
cared much about 64-bit architectures.  It has a hole in the middle.
And this hole is creating an information leak in the sigaltstack
syscall.

When the second parameter to sigaltstack is non-NULL the current stack
information is passed to the caller by filling in the members of the
stack_t structure and then the whole structure is copied using
copy_to_user.  This unfortunately leaves the whole after flags
uninitialized.

The following patch should fix the issue.

Signed-off-by: Ulrich Drepper <drepper@redhat.com>


diff --git a/kernel/signal.c b/kernel/signal.c
index ccf1cee..612d6b7 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2455,6 +2455,9 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
 	int error;
 
 	if (uoss) {
+		if (offsetof(stack_t, ss_flags) + sizeof(oss.ss_flags) !=
+		    offsetof(stack_t, ss_size))
+			memset(&oss, '\0', sizeof(oss));
 		oss.ss_sp = (void __user *) current->sas_ss_sp;
 		oss.ss_size = current->sas_ss_size;
 		oss.ss_flags = sas_ss_flags(sp);

Re: [PATCH] information leak in sigaltstack

[Linus Torvalds]

On Fri, 31 Jul 2009, Ulrich Drepper wrote:
> 
> The following patch should fix the issue.

Hmm. Is there any reason not to do an unconditional memset(), and then 
expect gcc to avoid the unnecessary stores? I realize gcc may not do that, 
but we could always _hope_.

Also, is there really any reason to believe that the only hole can be 
after ss_flags, and that it's only the case when ss_flags is in the 
middle? Quite frankly, as far as I can tell, you could have an "int 
ss_flags" at the _end_ of the structure too, and have the same issue 
(padding out to the alignment of the struct).

For an example of that "'int ss_flags' at the end" look at MIPS.

Now, you'd end up with a memset() in that case (since it certainly won't 
match the offsetof), but my point is, the conditional really looks very 
arbitrary and rather strange. I'd rather see it unconditional, even if it 
costs three unnecessary writes or whatever.

		Linus

Re: [PATCH] information leak in sigaltstack

[Linus Torvalds]

On Fri, 31 Jul 2009, Linus Torvalds wrote:
> 
> Now, you'd end up with a memset() in that case (since it certainly won't 
> match the offsetof), but my point is, the conditional really looks very 
> arbitrary and rather strange. I'd rather see it unconditional, even if it 
> costs three unnecessary writes or whatever.

.. and if we really do want the conditional, maybe just make it something 
like

	/*
	 * ss_flags is often generally 'int', and may cause
	 * holes in the structure due to alignment.
	 */
	if (alignof(oss.ss_flags) != alignof(oss))
		memset(&oss, 0, sizeof(oss));

instead? That would seem to be less subtle, and more to-the-point.

			Linus

Re: [PATCH] information leak in sigaltstack

[Ulrich Drepper]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Linus Torvalds wrote:

> 	if (alignof(oss.ss_flags) != alignof(oss))
> 		memset(&oss, 0, sizeof(oss));
> 
> instead? That would seem to be less subtle, and more to-the-point.

I was just composing a reply with basically this.  So  you'll apply this
and don't wait for me to send a new version of the patch, right?

- --
➧ Ulrich Drepper ➧ Red Hat, Inc. ➧ 444 Castro St ➧ Mountain View, CA ❖
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAkpzZBwACgkQ2ijCOnn/RHSMUACeJkKaHTG97OcSkCa3jFCIFxvL
9JsAoK7IuTCiW+i1oR9Xlt3wo0YZPTCw
=9A0Y
-----END PGP SIGNATURE-----

Re: [PATCH] information leak in sigaltstack

[Linus Torvalds]

[ Cc'ing jakub, since that code generation looks crappy, and I think he 
  has worked on gcc memset(). I wonder if it's because we use -Os, and gcc 
  tries to avoid one REX prefix on the 'stosq'.

  I also wonder why gcc doesn't just notice that it should really only 
  initialize a single 4-byte word (no rep, no prefix, no nothing, just a 
  single "movl $0,44(%ebp)") - so even with the -Os, that is just wrong, 
  and it would have been better to do as multiple stores and then noticing 
  that most of them end up dead ]

On Fri, 31 Jul 2009, Ulrich Drepper wrote:
> 
> I was just composing a reply with basically this.  So  you'll apply this
> and don't wait for me to send a new version of the patch, right?

Grr. Gcc creates truly crap code for this trivial 24-byte memset. Why does 
it do that?

gcc knows the alignment is 8 bytes, but it still uses 6 4-byte stores 
instead of 3 8-byte ones. And it does it with this:

        xorl    %eax, %eax      # tmp88
        leaq    -48(%rbp), %rsi #, tmp86
        movl    $6, %ecx        #, tmp89
        movq    %rsi, %rdi      # tmp86, tmp87
        rep stosl

which is just incredibly lame in so many ways.

And it doesn't optimize anything away, even though the next lines will 
then re-initialize 20 of the 24 bytes.

Now, maybe this isn't performance-critical, but it just makes me feel that 
there has to be a better way to make gcc DTRT. 

Here's the patch I used, just for posterity. I can't decide if I really 
want to commit this crap. But at least on 32-bit architectures the 
"alignof" testing should remove the horrid code. I do wonder why gcc 
thinks that 32-bit writes are a good idea in this case, though.

		Linus

---
 kernel/signal.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index ccf1cee..b990dc8 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2455,6 +2455,9 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
 	int error;
 
 	if (uoss) {
+		/* Fill cracks around 'ss_flags' */
+		if (__alignof__(oss.ss_flags) != __alignof__(oss))
+			memset(&oss, 0, sizeof(oss));
 		oss.ss_sp = (void __user *) current->sas_ss_sp;
 		oss.ss_size = current->sas_ss_size;
 		oss.ss_flags = sas_ss_flags(sp);

Re: [PATCH] information leak in sigaltstack

[Jakub Jelinek]

On Fri, Jul 31, 2009 at 05:28:40PM -0700, Linus Torvalds wrote:
> 
> [ Cc'ing jakub, since that code generation looks crappy, and I think he 
>   has worked on gcc memset(). I wonder if it's because we use -Os, and gcc 
>   tries to avoid one REX prefix on the 'stosq'.

Assuming we are talking about:
typedef __SIZE_TYPE__ size_t;
typedef struct sigaltstack { void *ss_sp; int ss_flags; size_t ss_size; }
stack_t;
void foo (stack_t *oss, void *sp)
{
  __builtin_memset (oss, 0, sizeof (*oss));
  oss->ss_sp = sp;
  oss->ss_flags = 6;
  oss->ss_size = 2; 
}

yes, it is because of -Os, rep stosq is longer than rep stosl.  For -O2 it
generates:
        movq    $0, 8(%rdi)
        movq    %rsi, (%rdi)
        movl    $6, 8(%rdi)
        movq    $2, 16(%rdi)
which still isn't perfect, but is much better.  GCC 4.4 newly has RTL DCE
improvements, so that at least the memcpy is optimized away if the structure
is filled completely after the memset, or is able to optimize away NULL/0
assignments after a memset to 0.  At -O2 when GCC decides to do the memset
piecewise it is easier to kill dead stores from the memset (the -O2 code
perhaps could be improved by the http://gcc.gnu.org/PR22141 patch which
hasn't been committed because it didn't show visible improvements and on
power6 even degraded performance).  At -Os when GCC decides during the
expand to use arch specific pattern for the memset it would be much harder
to handle it at the RTL level.  So the above should be ideally optimized
already at the tree level.

> diff --git a/kernel/signal.c b/kernel/signal.c
> index ccf1cee..b990dc8 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2455,6 +2455,9 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
>  	int error;
>  
>  	if (uoss) {
> +		/* Fill cracks around 'ss_flags' */
> +		if (__alignof__(oss.ss_flags) != __alignof__(oss))
> +			memset(&oss, 0, sizeof(oss));
>  		oss.ss_sp = (void __user *) current->sas_ss_sp;
>  		oss.ss_size = current->sas_ss_size;
>  		oss.ss_flags = sas_ss_flags(sp);

I'd say the test you want to do is
if (sizeof (oss.ss_sp) + sizeof (oss.ss_size) + sizeof (oss.ss_flags)
    != sizeof oss)
  memset (&oss, 0, sizeof oss);
(i.e. check whether the struct has any padding in it or not).

	Jakub

Re: [PATCH] information leak in sigaltstack

[Linus Torvalds]

On Sat, 1 Aug 2009, Jakub Jelinek wrote:
> 
> yes, it is because of -Os, rep stosq is longer than rep stosl.  For -O2 it
> generates:
>         movq    $0, 8(%rdi)
>         movq    %rsi, (%rdi)
>         movl    $6, 8(%rdi)
>         movq    $2, 16(%rdi)

Ok, so -Os actually generates _larger_ code due to this silly one-byte 
micro-optimization that then disables other optimizations.

I suspect that the "rep stosl" choice is _particularly_ bad, since doing a 
32-bit write actually is much more likely to cause subsequent stalls on 
the store buffer if any of the latter accesses are 64-bit reads. It tends 
to be much better to create larger stores (and equal or smaller loads) if 
there are any overlapping accesses.

I think a lot of micro-architectures will stall if they try to do a read 
that crosses multiple stores in the store buffer. If the load hits in the 
store buffer and is entirely contained within one store, you have a much 
better chance of just bypassing the cache entirely.

> which still isn't perfect, but is much better.

Yes. It would have been nice if gcc optimized the overlapping accesses 
too, but it's already much better.

> At -O2 when GCC decides to do the memset piecewise it is easier to kill 
> dead stores from the memset

On 32-bit, we do memcpy() by hand because gcc is/was so bad at this (we do 
it for memset too, but only for really small areas, so it wouldn't 
trigger).

On x86-64, we've trusted that gcc is better. Sadly, it's clearly not very 
good. I bet that for anything that is just a couple of stores (three, in 
this case), you'd almost always be better off using regular stores. The 
"rep stosl" may be small, but the register constraints and the inability 
to combine memory accesses are a big downer.

> At -Os when GCC decides during the expand to use arch specific pattern 
> for the memset it would be much harder to handle it at the RTL level.  
> So the above should be ideally optimized already at the tree level.

Yes. Or perhaps by just saying that for -Os you'd still expand it for a 
really _small_ number of accesses (just make it smaller than for -O2).

> I'd say the test you want to do is
> if (sizeof (oss.ss_sp) + sizeof (oss.ss_size) + sizeof (oss.ss_flags)
>     != sizeof oss)
>   memset (&oss, 0, sizeof oss);
> (i.e. check whether the struct has any padding in it or not).

Yeah, that sounds logical. I'd still like to figure out how to get gcc to 
generate better code in this case, though. 

Explicit padding bytes would do it, of course, but then we're talking 
changs to every single 64-bit architecture. Grr.

			Linus

Re: [PATCH] information leak in sigaltstack

[Linus Torvalds]

On Fri, 31 Jul 2009, Linus Torvalds wrote:
> 
> Here's the patch I used, just for posterity. I can't decide if I really 
> want to commit this crap.

I think I like this version better. It's not wonderfully pretty, but 
to some degree it actually improves on code generation, even if it 
generates a bigger exception table. It now also copies the stack_t back to 
user space the same way it copies it _from_ user space, so in that sense 
it's actually very consistent.

		Linus
---
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 1 Aug 2009 10:34:56 -0700
Subject: [PATCH] do_sigaltstack: avoid copying 'stack_t' as a structure to user space

Ulrich Drepper correctly points out that there is generally padding in
the structure on 64-bit hosts, and that copying the structure from
kernel to user space can leak information from the kernel stack in those
padding bytes.

Avoid the whole issue by just copying the three members one by one
instead, which also means that the function also can avoid the need for
a stack frame.  This also happens to match how we copy the new structure
from user space, so it all even makes sense.

[ The obvious solution of adding a memset() generates horrid code, gcc
  does really stupid things. ]

Reported-by: Ulrich Drepper <drepper@redhat.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 kernel/signal.c |   15 ++++++++-------
 1 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/signal.c b/kernel/signal.c
index ccf1cee..f268372 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2454,11 +2454,9 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
 	stack_t oss;
 	int error;
 
-	if (uoss) {
-		oss.ss_sp = (void __user *) current->sas_ss_sp;
-		oss.ss_size = current->sas_ss_size;
-		oss.ss_flags = sas_ss_flags(sp);
-	}
+	oss.ss_sp = (void __user *) current->sas_ss_sp;
+	oss.ss_size = current->sas_ss_size;
+	oss.ss_flags = sas_ss_flags(sp);
 
 	if (uss) {
 		void __user *ss_sp;
@@ -2501,13 +2499,16 @@ do_sigaltstack (const stack_t __user *uss, stack_t __user *uoss, unsigned long s
 		current->sas_ss_size = ss_size;
 	}
 
+	error = 0;
 	if (uoss) {
 		error = -EFAULT;
-		if (copy_to_user(uoss, &oss, sizeof(oss)))
+		if (!access_ok(VERIFY_WRITE, uoss, sizeof(*uoss)))
 			goto out;
+		error = __put_user(oss.ss_sp, &uoss->ss_sp) |
+			__put_user(oss.ss_size, &uoss->ss_size) |
+			__put_user(oss.ss_flags, &uoss->ss_flags);
 	}
 
-	error = 0;
 out:
 	return error;
 }