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 <[email protected]>
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);
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
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
-----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-----
[ 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);