Messages in this thread Patch in this message | | | Date | Fri, 31 Jul 2009 17:28:40 -0700 (PDT) | From | Linus Torvalds <> | Subject | Re: [PATCH] information leak in sigaltstack |
| |
[ 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);
| |