| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090205233057.GA19561@elte.hu>
Date: Fri, 6 Feb 2009 00:30:57 +0100
From: Ingo Molnar <mingo@...e.hu>
To: Andi Kleen <andi@...stfloor.org>
Cc: gregkh@...e.de, ak@...ux.intel.com, hpa@...or.com,
stable@...nel.org, stable-commits@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: patch x86-use-early-clobbers-in-usercopy-.c.patch added to
2.6.28-stable tree
* Andi Kleen <andi@...stfloor.org> wrote:
> On Thu, Feb 05, 2009 at 07:09:44PM +0100, Ingo Molnar wrote:
> >
> > * Ingo Molnar <mingo@...e.hu> wrote:
> >
> > >
> > > * Andi Kleen <andi@...stfloor.org> wrote:
> > >
> > > > Just a quick comment.
> > > > >
> > > > > Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> > > > > Signed-off-by: H. Peter Anvin <hpa@...or.com>
> > > > > [ since this API is rarely used and no in-kernel user relies on a 'len'
> > > > > return value (they only rely on negative return values) this miscompile
> > > > > was never noticed in the field. But it's worth fixing it nevertheless. ]
> > > >
> > > > This comment is not correct, the problem caused really non booting kernels
> > > > for Hugh (although in somewhat exotic configurations). do_mount relies on
> > > > it.
> > >
> > > Where's the URL for the crash and the config? Why wasnt that info in the
> > > patch? There's no public discussion that i can find either.
> >
> > Ping? You complained about something but you didnt reply to my question.
>
> Sorry I was travelling and behind on mail.
>
> The report was in private mail which I'm not comfortable to forward.
Nobody asked you to forward private emails. All that was needed was the
impact information which you already shared above, that the bug caused a
non-booting kernel for Hugh.
As a contributor of a Linux kernel patch it is one of your duties to
describe practical impact. You decided to not come forward with that
information in your submission, and then you complain that your vague
changelog was misinterpreted by us - fully knowing that changing a git
commit message in hindsight is not possible.
So given that the fix is upstream already and that it is queued up in
-stable (and rightly so) the only purpose of your complaint about the log
message can be that you wanted to create some petty controversy about the
log message.
Doing so is your full right but you should know that actions like that
makes me trust you less every day. And in case you didnt notice, regarding
your whining in your re-submission [attached below]:
> x86@...nel.org seems to be the usual blackhole and ignored it.
The delay we have in processing your patches (5 days in this case) is
caused by your unwillingness to communicate and work efficiently with
the x86 maintainers.
You have the track record of a high-cost contributor: you frequently argue
about nonsensical, backwards looking issues (like here), you frequently
resist requests for changes to patches, you withhold information and you
stubbornly refuse to follow established contribution practices.
All in one, you expect to be treated in a special way, but in reality you
are not special, and the high cost and difficulty of communicating and
working with you makes us put you to the end of our TODO list.
Ingo
--------------------------------------->
Date: Wed, 21 Jan 2009 06:31:10 +0100
From: Andi Kleen <andi@...stfloor.org>
To: linux-kernel@...r.kernel.org, x86@...nel.org, akpm@...l.org
Subject: [RESEND] [PATCH] Use early clobbers in usercopy_64.c
Cc: hugh@...itas.com
Andrew, can you please merge this patch?
x86@...nel.org seems to be the usual blackhole and ignored it.
It's a critical bugfix for both 2.6.29 and 2.6.28-stable
-Andi
commit 229689f6f129334446e48e777d12cbf365745283
Author: Andi Kleen <ak@...ux.intel.com>
Date: Wed Jan 14 07:50:57 2009 +0100
Use early clobbers in usercopy_*.c
Hugh Dickins noticed that strncpy_from_user() was miscompiled
in some circumstances with gcc 4.3.
Thanks to Hugh's excellent analysis it was easy to track down.
Hugh writes:
Try building an x86_64 defconfig 2.6.29-rc1 kernel tree,
except not quite defconfig, switch CONFIG_PREEMPT_NONE=y
and CONFIG_PREEMPT_VOLUNTARY off (because it expands a
might_fault() there, which hides the issue): using a
gcc 4.3.2 (I've checked both openSUSE 11.1 and Fedora 10).
It generates the following:
0000000000000000 <__strncpy_from_user>:
0: 48 89 d1 mov %rdx,%rcx
3: 48 85 c9 test %rcx,%rcx
6: 74 0e je 16 <__strncpy_from_user+0x16>
8: ac lods %ds:(%rsi),%al
9: aa stos %al,%es:(%rdi)
a: 84 c0 test %al,%al
c: 74 05 je 13 <__strncpy_from_user+0x13>
e: 48 ff c9 dec %rcx
11: 75 f5 jne 8 <__strncpy_from_user+0x8>
13: 48 29 c9 sub %rcx,%rcx
16: 48 89 c8 mov %rcx,%rax
19: c3 retq
Observe that "sub %rcx,%rcx; mov %rcx,%rax", whereas gcc 4.2.1
(and many other configs) say "sub %rcx,%rdx; mov %rdx,%rax".
Isn't it returning 0 when it ought to be returning strlen?
AK:
The asm constraints for the strncpy_from_user() result were missing an
early clobber, which tells gcc that the last output arguments
are written before all input arguments are read.
I also added some more early clobbers in the rest of the file.
And indeed 32bit usercopy.c had the same problem in some places.
Fixed there too.
Signed-off-by: Andi Kleen <ak@...ux.intel.com>
diff --git a/arch/x86/lib/usercopy_32.c b/arch/x86/lib/usercopy_32.c
index 4a20b2f..7c8ca91 100644
--- a/arch/x86/lib/usercopy_32.c
+++ b/arch/x86/lib/usercopy_32.c
@@ -56,7 +56,7 @@ do { \
" jmp 2b\n" \
".previous\n" \
_ASM_EXTABLE(0b,3b) \
- : "=d"(res), "=c"(count), "=&a" (__d0), "=&S" (__d1), \
+ : "=&d"(res), "=&c"(count), "=&a" (__d0), "=&S" (__d1), \
"=&D" (__d2) \
: "i"(-EFAULT), "0"(count), "1"(count), "3"(src), "4"(dst) \
: "memory"); \
@@ -218,7 +218,7 @@ long strnlen_user(const char __user *s, long n)
" .align 4\n"
" .long 0b,2b\n"
".previous"
- :"=r" (n), "=D" (s), "=a" (res), "=c" (tmp)
+ :"=&r" (n), "=&D" (s), "=&a" (res), "=&c" (tmp)
:"0" (n), "1" (s), "2" (0), "3" (mask)
:"cc");
return res & mask;
diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c
index 64d6c84..ec13cb5 100644
--- a/arch/x86/lib/usercopy_64.c
+++ b/arch/x86/lib/usercopy_64.c
@@ -32,7 +32,7 @@ do { \
" jmp 2b\n" \
".previous\n" \
_ASM_EXTABLE(0b,3b) \
- : "=r"(res), "=c"(count), "=&a" (__d0), "=&S" (__d1), \
+ : "=&r"(res), "=&c"(count), "=&a" (__d0), "=&S" (__d1), \
"=&D" (__d2) \
: "i"(-EFAULT), "0"(count), "1"(count), "3"(src), "4"(dst) \
: "memory"); \
@@ -86,7 +86,7 @@ unsigned long __clear_user(void __user *addr, unsigned long size)
".previous\n"
_ASM_EXTABLE(0b,3b)
_ASM_EXTABLE(1b,2b)
- : [size8] "=c"(size), [dst] "=&D" (__d0)
+ : [size8] "=&c"(size), [dst] "=&D" (__d0)
: [size1] "r"(size & 7), "[size8]" (size / 8), "[dst]"(addr),
[zero] "r" (0UL), [eight] "r" (8UL));
return size;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists