Hi,
This is a re-send of an earlier patch series that got side-tracked. I'd
like to get this into the kernel in some form, so I'd like to try
submission again, as I think it is important.
Intel CPUs have an additional MSR bit to indicate if the BIOS was
configured to disable NX. This bit was traditionally used for operating
systems that did not understand how to handle the NX bit. Since Linux
understands this, this BIOS flag should be ignored by default.
In a review[1] of reported hardware being used by Ubuntu bug reporters,
almost 10% of systems had an incorrectly configured BIOS, leaving their
systems unable to use the NX features of their CPU.
This change will clear the MSR_IA32_MISC_ENABLE_XD_DISABLE bit so that NX
cannot be inappropriately controlled by the BIOS on Intel CPUs. If, under
very strange hardware configurations, NX actually needs to be disabled,
"noexec=off" can be used to restore the prior behavior.
Based on feedback from HPA, this was reworked to extend the existing
"verify_cpu" routines, and to more tightly confine which CPUs will call
MSR_IA32_MISC_ENABLE. Since it includes some re-arrangements of files, I
tried to break the patches up into their logical steps.
-Kees
[1] http://www.outflux.net/blog/archives/2010/02/18/data-mining-for-nx-bit/
--
Kees Cook
Ubuntu Security Team
This file does not need the _64 extension since the code is 32bit already,
and can be used in 32bit routines.
Signed-off-by: Kees Cook <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 2 +-
arch/x86/kernel/trampoline_64.S | 2 +-
arch/x86/kernel/verify_cpu.S | 106 ++++++++++++++++++++++++++++++++++++
arch/x86/kernel/verify_cpu_64.S | 106 ------------------------------------
4 files changed, 108 insertions(+), 108 deletions(-)
create mode 100644 arch/x86/kernel/verify_cpu.S
delete mode 100644 arch/x86/kernel/verify_cpu_64.S
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 52f85a1..35af09d 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -182,7 +182,7 @@ no_longmode:
hlt
jmp 1b
-#include "../../kernel/verify_cpu_64.S"
+#include "../../kernel/verify_cpu.S"
/*
* Be careful here startup_64 needs to be at a predictable
diff --git a/arch/x86/kernel/trampoline_64.S b/arch/x86/kernel/trampoline_64.S
index 3af2dff..075d130 100644
--- a/arch/x86/kernel/trampoline_64.S
+++ b/arch/x86/kernel/trampoline_64.S
@@ -127,7 +127,7 @@ startup_64:
no_longmode:
hlt
jmp no_longmode
-#include "verify_cpu_64.S"
+#include "verify_cpu.S"
# Careful these need to be in the same 64K segment as the above;
tidt:
diff --git a/arch/x86/kernel/verify_cpu.S b/arch/x86/kernel/verify_cpu.S
new file mode 100644
index 0000000..56a8c2a
--- /dev/null
+++ b/arch/x86/kernel/verify_cpu.S
@@ -0,0 +1,106 @@
+/*
+ *
+ * verify_cpu.S - Code for cpu long mode and SSE verification. This
+ * code has been borrowed from boot/setup.S and was introduced by
+ * Andi Kleen.
+ *
+ * Copyright (c) 2007 Andi Kleen ([email protected])
+ * Copyright (c) 2007 Eric Biederman ([email protected])
+ * Copyright (c) 2007 Vivek Goyal ([email protected])
+ *
+ * This source code is licensed under the GNU General Public License,
+ * Version 2. See the file COPYING for more details.
+ *
+ * This is a common code for verification whether CPU supports
+ * long mode and SSE or not. It is not called directly instead this
+ * file is included at various places and compiled in that context.
+ * Following are the current usage.
+ *
+ * This file is included by both 16bit and 32bit code.
+ *
+ * arch/x86_64/boot/setup.S : Boot cpu verification (16bit)
+ * arch/x86_64/boot/compressed/head.S: Boot cpu verification (32bit)
+ * arch/x86_64/kernel/trampoline.S: secondary processor verfication (16bit)
+ * arch/x86_64/kernel/acpi/wakeup.S:Verfication at resume (16bit)
+ *
+ * verify_cpu, returns the status of cpu check in register %eax.
+ * 0: Success 1: Failure
+ *
+ * The caller needs to check for the error code and take the action
+ * appropriately. Either display a message or halt.
+ */
+
+#include <asm/cpufeature.h>
+#include <asm/msr-index.h>
+
+verify_cpu:
+ pushfl # Save caller passed flags
+ pushl $0 # Kill any dangerous flags
+ popfl
+
+ pushfl # standard way to check for cpuid
+ popl %eax
+ movl %eax,%ebx
+ xorl $0x200000,%eax
+ pushl %eax
+ popfl
+ pushfl
+ popl %eax
+ cmpl %eax,%ebx
+ jz verify_cpu_no_longmode # cpu has no cpuid
+
+ movl $0x0,%eax # See if cpuid 1 is implemented
+ cpuid
+ cmpl $0x1,%eax
+ jb verify_cpu_no_longmode # no cpuid 1
+
+ xor %di,%di
+ cmpl $0x68747541,%ebx # AuthenticAMD
+ jnz verify_cpu_noamd
+ cmpl $0x69746e65,%edx
+ jnz verify_cpu_noamd
+ cmpl $0x444d4163,%ecx
+ jnz verify_cpu_noamd
+ mov $1,%di # cpu is from AMD
+
+verify_cpu_noamd:
+ movl $0x1,%eax # Does the cpu have what it takes
+ cpuid
+ andl $REQUIRED_MASK0,%edx
+ xorl $REQUIRED_MASK0,%edx
+ jnz verify_cpu_no_longmode
+
+ movl $0x80000000,%eax # See if extended cpuid is implemented
+ cpuid
+ cmpl $0x80000001,%eax
+ jb verify_cpu_no_longmode # no extended cpuid
+
+ movl $0x80000001,%eax # Does the cpu have what it takes
+ cpuid
+ andl $REQUIRED_MASK1,%edx
+ xorl $REQUIRED_MASK1,%edx
+ jnz verify_cpu_no_longmode
+
+verify_cpu_sse_test:
+ movl $1,%eax
+ cpuid
+ andl $SSE_MASK,%edx
+ cmpl $SSE_MASK,%edx
+ je verify_cpu_sse_ok
+ test %di,%di
+ jz verify_cpu_no_longmode # only try to force SSE on AMD
+ movl $MSR_K7_HWCR,%ecx
+ rdmsr
+ btr $15,%eax # enable SSE
+ wrmsr
+ xor %di,%di # don't loop
+ jmp verify_cpu_sse_test # try again
+
+verify_cpu_no_longmode:
+ popfl # Restore caller passed flags
+ movl $1,%eax
+ ret
+verify_cpu_sse_ok:
+ popfl # Restore caller passed flags
+ xorl %eax, %eax
+ ret
diff --git a/arch/x86/kernel/verify_cpu_64.S b/arch/x86/kernel/verify_cpu_64.S
deleted file mode 100644
index 56a8c2a..0000000
--- a/arch/x86/kernel/verify_cpu_64.S
+++ /dev/null
@@ -1,106 +0,0 @@
-/*
- *
- * verify_cpu.S - Code for cpu long mode and SSE verification. This
- * code has been borrowed from boot/setup.S and was introduced by
- * Andi Kleen.
- *
- * Copyright (c) 2007 Andi Kleen ([email protected])
- * Copyright (c) 2007 Eric Biederman ([email protected])
- * Copyright (c) 2007 Vivek Goyal ([email protected])
- *
- * This source code is licensed under the GNU General Public License,
- * Version 2. See the file COPYING for more details.
- *
- * This is a common code for verification whether CPU supports
- * long mode and SSE or not. It is not called directly instead this
- * file is included at various places and compiled in that context.
- * Following are the current usage.
- *
- * This file is included by both 16bit and 32bit code.
- *
- * arch/x86_64/boot/setup.S : Boot cpu verification (16bit)
- * arch/x86_64/boot/compressed/head.S: Boot cpu verification (32bit)
- * arch/x86_64/kernel/trampoline.S: secondary processor verfication (16bit)
- * arch/x86_64/kernel/acpi/wakeup.S:Verfication at resume (16bit)
- *
- * verify_cpu, returns the status of cpu check in register %eax.
- * 0: Success 1: Failure
- *
- * The caller needs to check for the error code and take the action
- * appropriately. Either display a message or halt.
- */
-
-#include <asm/cpufeature.h>
-#include <asm/msr-index.h>
-
-verify_cpu:
- pushfl # Save caller passed flags
- pushl $0 # Kill any dangerous flags
- popfl
-
- pushfl # standard way to check for cpuid
- popl %eax
- movl %eax,%ebx
- xorl $0x200000,%eax
- pushl %eax
- popfl
- pushfl
- popl %eax
- cmpl %eax,%ebx
- jz verify_cpu_no_longmode # cpu has no cpuid
-
- movl $0x0,%eax # See if cpuid 1 is implemented
- cpuid
- cmpl $0x1,%eax
- jb verify_cpu_no_longmode # no cpuid 1
-
- xor %di,%di
- cmpl $0x68747541,%ebx # AuthenticAMD
- jnz verify_cpu_noamd
- cmpl $0x69746e65,%edx
- jnz verify_cpu_noamd
- cmpl $0x444d4163,%ecx
- jnz verify_cpu_noamd
- mov $1,%di # cpu is from AMD
-
-verify_cpu_noamd:
- movl $0x1,%eax # Does the cpu have what it takes
- cpuid
- andl $REQUIRED_MASK0,%edx
- xorl $REQUIRED_MASK0,%edx
- jnz verify_cpu_no_longmode
-
- movl $0x80000000,%eax # See if extended cpuid is implemented
- cpuid
- cmpl $0x80000001,%eax
- jb verify_cpu_no_longmode # no extended cpuid
-
- movl $0x80000001,%eax # Does the cpu have what it takes
- cpuid
- andl $REQUIRED_MASK1,%edx
- xorl $REQUIRED_MASK1,%edx
- jnz verify_cpu_no_longmode
-
-verify_cpu_sse_test:
- movl $1,%eax
- cpuid
- andl $SSE_MASK,%edx
- cmpl $SSE_MASK,%edx
- je verify_cpu_sse_ok
- test %di,%di
- jz verify_cpu_no_longmode # only try to force SSE on AMD
- movl $MSR_K7_HWCR,%ecx
- rdmsr
- btr $15,%eax # enable SSE
- wrmsr
- xor %di,%di # don't loop
- jmp verify_cpu_sse_test # try again
-
-verify_cpu_no_longmode:
- popfl # Restore caller passed flags
- movl $1,%eax
- ret
-verify_cpu_sse_ok:
- popfl # Restore caller passed flags
- xorl %eax, %eax
- ret
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
This change will clear the MSR_IA32_MISC_ENABLE_XD_DISABLE bit so that NX
cannot be inappropriately controlled by the BIOS on Intel CPUs. If, under
very strange hardware configurations, NX actually needs to be disabled,
"noexec=off" can be used to restore the prior behavior.
Signed-off-by: Kees Cook <[email protected]>
---
arch/x86/kernel/verify_cpu.S | 48 +++++++++++++++++++++++++++++++++++-------
1 files changed, 40 insertions(+), 8 deletions(-)
diff --git a/arch/x86/kernel/verify_cpu.S b/arch/x86/kernel/verify_cpu.S
index 56a8c2a..ccb4136 100644
--- a/arch/x86/kernel/verify_cpu.S
+++ b/arch/x86/kernel/verify_cpu.S
@@ -7,6 +7,7 @@
* Copyright (c) 2007 Andi Kleen ([email protected])
* Copyright (c) 2007 Eric Biederman ([email protected])
* Copyright (c) 2007 Vivek Goyal ([email protected])
+ * Copyright (c) 2010 Kees Cook ([email protected])
*
* This source code is licensed under the GNU General Public License,
* Version 2. See the file COPYING for more details.
@@ -14,18 +15,16 @@
* This is a common code for verification whether CPU supports
* long mode and SSE or not. It is not called directly instead this
* file is included at various places and compiled in that context.
- * Following are the current usage.
+ * This file is expected to run in 32bit code. Currently:
*
- * This file is included by both 16bit and 32bit code.
+ * arch/x86_64/boot/compressed/head_64.S: Boot cpu verification
+ * arch/x86_64/kernel/trampoline_64.S: secondary processor verfication
*
- * arch/x86_64/boot/setup.S : Boot cpu verification (16bit)
- * arch/x86_64/boot/compressed/head.S: Boot cpu verification (32bit)
- * arch/x86_64/kernel/trampoline.S: secondary processor verfication (16bit)
- * arch/x86_64/kernel/acpi/wakeup.S:Verfication at resume (16bit)
- *
- * verify_cpu, returns the status of cpu check in register %eax.
+ * verify_cpu, returns the status of longmode and SSE in register %eax.
* 0: Success 1: Failure
*
+ * On Intel, the XD_DISABLE flag will be cleared as a side-effect.
+ *
* The caller needs to check for the error code and take the action
* appropriately. Either display a message or halt.
*/
@@ -62,8 +61,41 @@ verify_cpu:
cmpl $0x444d4163,%ecx
jnz verify_cpu_noamd
mov $1,%di # cpu is from AMD
+ jmp verify_cpu_check
verify_cpu_noamd:
+ cmpl $0x756e6547,%ebx # GenuineIntel?
+ jnz verify_cpu_check
+ cmpl $0x49656e69,%edx
+ jnz verify_cpu_check
+ cmpl $0x6c65746e,%ecx
+ jnz verify_cpu_check
+
+ # only call IA32_MISC_ENABLE when:
+ # family > 6 || (family == 6 && model >= 0xd)
+ movl $0x1, %eax # check CPU family and model
+ cpuid
+ movl %eax, %ecx
+
+ andl $0x0ff00f00, %eax # mask family and extended family
+ shrl $8, %eax
+ cmpl $6, %eax
+ ja verify_cpu_clear_xd # family > 6, ok
+ jb verify_cpu_check # family < 6, skip
+
+ andl $0x000f00f0, %ecx # mask model and extended model
+ shrl $4, %ecx
+ cmpl $0xd, %ecx
+ jb verify_cpu_check # family == 6, model < 0xd, skip
+
+verify_cpu_clear_xd:
+ movl $MSR_IA32_MISC_ENABLE, %ecx
+ rdmsr
+ btrl $2, %edx # clear MSR_IA32_MISC_ENABLE_XD_DISABLE
+ jnc verify_cpu_check # only write MSR if bit was changed
+ wrmsr
+
+verify_cpu_check:
movl $0x1,%eax # Does the cpu have what it takes
cpuid
andl $REQUIRED_MASK0,%edx
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
The XD_DISABLE-clearing side-effect needs to happen on 32bit CPU
start-up as well.
Signed-off-by: Kees Cook <[email protected]>
---
arch/x86/kernel/head_32.S | 6 ++++++
arch/x86/kernel/verify_cpu.S | 1 +
2 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index bcece91..fdaea52 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -314,6 +314,10 @@ ENTRY(startup_32_smp)
subl $0x80000001, %eax
cmpl $(0x8000ffff-0x80000001), %eax
ja 6f
+
+ /* Clear bogus XD_DISABLE bits */
+ call verify_cpu
+
mov $0x80000001, %eax
cpuid
/* Execute Disable bit supported? */
@@ -609,6 +613,8 @@ ignore_int:
#endif
iret
+#include "verify_cpu.S"
+
__REFDATA
.align 4
ENTRY(initial_code)
diff --git a/arch/x86/kernel/verify_cpu.S b/arch/x86/kernel/verify_cpu.S
index ccb4136..5644b4b 100644
--- a/arch/x86/kernel/verify_cpu.S
+++ b/arch/x86/kernel/verify_cpu.S
@@ -19,6 +19,7 @@
*
* arch/x86_64/boot/compressed/head_64.S: Boot cpu verification
* arch/x86_64/kernel/trampoline_64.S: secondary processor verfication
+ * arch/x86_64/kernel/head_32.S: processor startup
*
* verify_cpu, returns the status of longmode and SSE in register %eax.
* 0: Success 1: Failure
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
Fix the NX feature boot warning when NX is missing to correctly
reflect that BIOSes cannot disable NX now.
Signed-off-by: Kees Cook <[email protected]>
---
arch/x86/mm/setup_nx.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index a3250aa..410531d 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -41,7 +41,7 @@ void __init x86_report_nx(void)
{
if (!cpu_has_nx) {
printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
- "missing in CPU or disabled in BIOS!\n");
+ "missing in CPU!\n");
} else {
#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
if (disable_nx) {
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
> In a review[1] of reported hardware being used by Ubuntu bug reporters,
> almost 10% of systems had an incorrectly configured BIOS, leaving their
> systems unable to use the NX features of their CPU.
Ouch
> This change will clear the MSR_IA32_MISC_ENABLE_XD_DISABLE bit so that NX
> cannot be inappropriately controlled by the BIOS on Intel CPUs. If, under
> very strange hardware configurations, NX actually needs to be disabled,
> "noexec=off" can be used to restore the prior behavior.
Have you done an audit of CPU errata to ensure that none of these cases
are ones where the BIOS has disabled it to avoid an erratum. I'd hate to
turn it on and find out the BIOS actually turned it off for good reason !
Alan
On Tue, Nov 9, 2010 at 8:14 PM, Kees Cook <[email protected]> wrote:
> This file does not need the _64 extension since the code is 32bit already,
> and can be used in 32bit routines.
>
> Signed-off-by: Kees Cook <[email protected]>
> ---
> ?arch/x86/boot/compressed/head_64.S | ? ?2 +-
> ?arch/x86/kernel/trampoline_64.S ? ?| ? ?2 +-
> ?arch/x86/kernel/verify_cpu.S ? ? ? | ?106 ++++++++++++++++++++++++++++++++++++
> ?arch/x86/kernel/verify_cpu_64.S ? ?| ?106 ------------------------------------
> ?4 files changed, 108 insertions(+), 108 deletions(-)
> ?create mode 100644 arch/x86/kernel/verify_cpu.S
> ?delete mode 100644 arch/x86/kernel/verify_cpu_64.S
Please use "git format-patch -M" to detect renames when preparing patches.
On Tue, Nov 09, 2010 at 06:31:42PM +0000, Alan Cox wrote:
> > In a review[1] of reported hardware being used by Ubuntu bug reporters,
> > almost 10% of systems had an incorrectly configured BIOS, leaving their
> > systems unable to use the NX features of their CPU.
>
> Ouch
Yeah :(
> > This change will clear the MSR_IA32_MISC_ENABLE_XD_DISABLE bit so that NX
> > cannot be inappropriately controlled by the BIOS on Intel CPUs. If, under
> > very strange hardware configurations, NX actually needs to be disabled,
> > "noexec=off" can be used to restore the prior behavior.
>
> Have you done an audit of CPU errata to ensure that none of these cases
> are ones where the BIOS has disabled it to avoid an erratum. I'd hate to
> turn it on and find out the BIOS actually turned it off for good reason !
Where can I find those details?
-Kees
--
Kees Cook
Ubuntu Security Team
The code is 32bit already, and can be used in 32bit routines.
Signed-off-by: Kees Cook <[email protected]>
---
arch/x86/boot/compressed/head_64.S | 2 +-
arch/x86/kernel/trampoline_64.S | 2 +-
arch/x86/kernel/{verify_cpu_64.S => verify_cpu.S} | 0
3 files changed, 2 insertions(+), 2 deletions(-)
rename arch/x86/kernel/{verify_cpu_64.S => verify_cpu.S} (100%)
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index 52f85a1..35af09d 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -182,7 +182,7 @@ no_longmode:
hlt
jmp 1b
-#include "../../kernel/verify_cpu_64.S"
+#include "../../kernel/verify_cpu.S"
/*
* Be careful here startup_64 needs to be at a predictable
diff --git a/arch/x86/kernel/trampoline_64.S b/arch/x86/kernel/trampoline_64.S
index 3af2dff..075d130 100644
--- a/arch/x86/kernel/trampoline_64.S
+++ b/arch/x86/kernel/trampoline_64.S
@@ -127,7 +127,7 @@ startup_64:
no_longmode:
hlt
jmp no_longmode
-#include "verify_cpu_64.S"
+#include "verify_cpu.S"
# Careful these need to be in the same 64K segment as the above;
tidt:
diff --git a/arch/x86/kernel/verify_cpu_64.S b/arch/x86/kernel/verify_cpu.S
similarity index 100%
rename from arch/x86/kernel/verify_cpu_64.S
rename to arch/x86/kernel/verify_cpu.S
--
1.7.2.3
--
Kees Cook
Ubuntu Security Team
Hi Pekka,
On Tue, Nov 09, 2010 at 08:46:42PM +0200, Pekka Enberg wrote:
> Please use "git format-patch -M" to detect renames when preparing patches.
Thanks, I've resent.
The proper use of format-patch should probably be covered in
Documentation/SubmittingPatches.txt but I didn't really see a good place
to put it.
Is there a ~/.gitconfig stanza that can be added to default to using -M
always?
Thanks,
-Kees
--
Kees Cook
Ubuntu Security Team
On Tue, Nov 9, 2010 at 8:15 PM, Kees Cook <[email protected]> wrote:
> The XD_DISABLE-clearing side-effect needs to happen on 32bit CPU
> start-up as well.
>
> Signed-off-by: Kees Cook <[email protected]>
The patch description here is pretty damn terse. Why do we need the
clearing for? Does not clearing XD_DISABLE cause some problem?
On Tue, Nov 9, 2010 at 9:02 PM, Kees Cook <[email protected]> wrote:
> Is there a ~/.gitconfig stanza that can be added to default to using -M
> always?
IIRC, setting "diff.renames" to "true" makes format-patch use "-M" by default.
On Tue, Nov 09, 2010 at 09:09:18PM +0200, Pekka Enberg wrote:
> On Tue, Nov 9, 2010 at 8:15 PM, Kees Cook <[email protected]> wrote:
> > The XD_DISABLE-clearing side-effect needs to happen on 32bit CPU
> > start-up as well.
> >
> > Signed-off-by: Kees Cook <[email protected]>
>
> The patch description here is pretty damn terse. Why do we need the
> clearing for? Does not clearing XD_DISABLE cause some problem?
The clearing needs to happen for both 32bit and 64bit, but the 32bit init
routines were not calling verify_cpu() yet. This adds that path to gain the
side-effect. (See patch 0 for why clearing XD_DISABLE is important.)
--
Kees Cook
Ubuntu Security Team
> On Tue, Nov 09, 2010 at 09:09:18PM +0200, Pekka Enberg wrote:
>> On Tue, Nov 9, 2010 at 8:15 PM, Kees Cook <[email protected]> wrote:
>> > The XD_DISABLE-clearing side-effect needs to happen on 32bit CPU
>> > start-up as well.
>> >
>> > Signed-off-by: Kees Cook <[email protected]>
>>
>> The patch description here is pretty damn terse. Why do we need the
>> clearing for? Does not clearing XD_DISABLE cause some problem?
On Tue, Nov 9, 2010 at 9:19 PM, Kees Cook <[email protected]> wrote:
> The clearing needs to happen for both 32bit and 64bit, but the 32bit init
> routines were not calling verify_cpu() yet. This adds that path to gain the
> side-effect. (See patch 0 for why clearing XD_DISABLE is important.)
I actually don't see 0/4 or 2/4 on LKML yet. It would be better to
have the rationale in the patch that changes the code for future
reference. The summary email will be lost in the noise much more
easily.
Pekka
On Tue, Nov 09, 2010 at 09:46:39PM +0200, Pekka Enberg wrote:
> I actually don't see 0/4 or 2/4 on LKML yet. It would be better to
> have the rationale in the patch that changes the code for future
> reference. The summary email will be lost in the noise much more
> easily.
Looks like delivery delays somewhere. I see it in the archive:
http://lkml.org/lkml/2010/11/9/380
But sure, I can improve the commit messages further.
-Kees
--
Kees Cook
Ubuntu Security Team
On Tue, Nov 9, 2010 at 9:00 PM, Kees Cook <[email protected]> wrote:
> The code is 32bit already, and can be used in 32bit routines.
>
> Signed-off-by: Kees Cook <[email protected]>
Acked-by: Pekka Enberg <[email protected]>
Hi Kees,
On Tue, Nov 09, 2010 at 09:46:39PM +0200, Pekka Enberg wrote:
>> I actually don't see 0/4 or 2/4 on LKML yet. It would be better to
>> have the rationale in the patch that changes the code for future
>> reference. The summary email will be lost in the noise much more
>> easily.
On Tue, Nov 9, 2010 at 9:56 PM, Kees Cook <[email protected]> wrote:
> Looks like delivery delays somewhere. I see it in the archive:
> http://lkml.org/lkml/2010/11/9/380
So why can't we do patch 2/4 XD_DISABLE clearing in
early_init_intel()? Why do we want to call verify_cpu() from
arch/x86/kernel/head_32.S and not from
arch/x86/boot/compressed/head_32.S like we do on 64-bit?
Pekka
Hi Pekka,
On Tue, Nov 09, 2010 at 10:28:16PM +0200, Pekka Enberg wrote:
> So why can't we do patch 2/4 XD_DISABLE clearing in
> early_init_intel()?
Because it's too late, unfortunately. I went around a few times about this
with Peter Anvin, and ultimately he agreed that it needed to go in
verify_cpu() for now.
> Why do we want to call verify_cpu() from
> arch/x86/kernel/head_32.S and not from
> arch/x86/boot/compressed/head_32.S like we do on 64-bit?
Because the longmode/SSE tests being performed in verify_cpu() need to
happen that early for 64bit. Instead of including it in two places for
32bit, we can just include it once in arch/x86/kernel/head_32.S.
-Kees
--
Kees Cook
Ubuntu Security Team
On 9.11.2010 22.48, Kees Cook wrote:
> Hi Pekka,
>
> On Tue, Nov 09, 2010 at 10:28:16PM +0200, Pekka Enberg wrote:
>> So why can't we do patch 2/4 XD_DISABLE clearing in
>> early_init_intel()?
> Because it's too late, unfortunately. I went around a few times about this
> with Peter Anvin, and ultimately he agreed that it needed to go in
> verify_cpu() for now.
OK, thanks for the explanation. A link to the previous discussion would
have been helpful here.
Acked-by: Pekka Enberg <[email protected]>
for the whole series.
Pekka
On Tue, 9 Nov 2010 10:56:04 -0800
Kees Cook <[email protected]> wrote:
> On Tue, Nov 09, 2010 at 06:31:42PM +0000, Alan Cox wrote:
> > > In a review[1] of reported hardware being used by Ubuntu bug reporters,
> > > almost 10% of systems had an incorrectly configured BIOS, leaving their
> > > systems unable to use the NX features of their CPU.
> >
> > Ouch
>
> Yeah :(
>
> > > This change will clear the MSR_IA32_MISC_ENABLE_XD_DISABLE bit so that NX
> > > cannot be inappropriately controlled by the BIOS on Intel CPUs. If, under
> > > very strange hardware configurations, NX actually needs to be disabled,
> > > "noexec=off" can be used to restore the prior behavior.
> >
> > Have you done an audit of CPU errata to ensure that none of these cases
> > are ones where the BIOS has disabled it to avoid an erratum. I'd hate to
> > turn it on and find out the BIOS actually turned it off for good reason !
>
> Where can I find those details?
In the errata manuals for each processor released by the relevant
vendors. As they are aggregated into families and they have an index in
the front it shouldn't take too long to check the ones your 10% scan
found.
I'm not specifically aware of any but I do know for example that there
are other CPU BIOS disablable features that some systems disable in the
BIOS for good reason (an ancient example being the original Pentium
REP MOVS optimisation on some steppings)
Hi Alan,
On Tue, Nov 09, 2010 at 10:50:00PM +0000, Alan Cox wrote:
> On Tue, 9 Nov 2010 10:56:04 -0800
> Kees Cook <[email protected]> wrote:
>
> > On Tue, Nov 09, 2010 at 06:31:42PM +0000, Alan Cox wrote:
> > > > In a review[1] of reported hardware being used by Ubuntu bug reporters,
> > > > almost 10% of systems had an incorrectly configured BIOS, leaving their
> > > > systems unable to use the NX features of their CPU.
> > >
> > > Ouch
> >
> > Yeah :(
> >
> > > > This change will clear the MSR_IA32_MISC_ENABLE_XD_DISABLE bit so that NX
> > > > cannot be inappropriately controlled by the BIOS on Intel CPUs. If, under
> > > > very strange hardware configurations, NX actually needs to be disabled,
> > > > "noexec=off" can be used to restore the prior behavior.
> > >
> > > Have you done an audit of CPU errata to ensure that none of these cases
> > > are ones where the BIOS has disabled it to avoid an erratum. I'd hate to
> > > turn it on and find out the BIOS actually turned it off for good reason !
> >
> > Where can I find those details?
>
> In the errata manuals for each processor released by the relevant
> vendors. As they are aggregated into families and they have an index in
> the front it shouldn't take too long to check the ones your 10% scan
> found.
Only Intel has this problem (it's the only CPU that defines this MSR), so
that'll reduce it. But Google is of no help to me on this; where can I find
these documents?
> I'm not specifically aware of any but I do know for example that there
> are other CPU BIOS disablable features that some systems disable in the
> BIOS for good reason (an ancient example being the original Pentium
> REP MOVS optimisation on some steppings)
By definition there should be fewer errata CPUs than those than need their
BIOS ignored, so I still think this patch makes sense (especially since it
can trivially be worked around with the noexec=off boot option).
-Kees
--
Kees Cook
Ubuntu Security Team
> Only Intel has this problem (it's the only CPU that defines this MSR), so
> that'll reduce it. But Google is of no help to me on this; where can I find
> these documents?
http://www.intel.com 8)
> > I'm not specifically aware of any but I do know for example that there
> > are other CPU BIOS disablable features that some systems disable in the
> > BIOS for good reason (an ancient example being the original Pentium
> > REP MOVS optimisation on some steppings)
>
> By definition there should be fewer errata CPUs than those than need their
> BIOS ignored, so I still think this patch makes sense (especially since it
> can trivially be worked around with the noexec=off boot option).
We need to be sure because the last thing you want is even 1% of your 10%
of users to sudden get magical random memory corruption from overriding
something wrongly !
It's definitely worth checking and worth doing because even if there is
one (and I have no idea if there is) it'll tell you which steppings
Alan
On Wed, Nov 10, 2010 at 12:21:53AM +0000, Alan Cox wrote:
> > Only Intel has this problem (it's the only CPU that defines this MSR), so
> > that'll reduce it. But Google is of no help to me on this; where can I find
> > these documents?
>
> http://www.intel.com 8)
Gee, thanks. :P
I looks like there is nothing that actually lists errata directly, so
walking the entire site for lists of all processor types, e.g.:
http://www.intel.com/products/processors/previousgeneration/index.htm
http://www.intel.com/design/celeron/documentation.htm
and then checking spec updates is the only way to go.
> > > I'm not specifically aware of any but I do know for example that there
> > > are other CPU BIOS disablable features that some systems disable in the
> > > BIOS for good reason (an ancient example being the original Pentium
> > > REP MOVS optimisation on some steppings)
> >
> > By definition there should be fewer errata CPUs than those than need their
> > BIOS ignored, so I still think this patch makes sense (especially since it
> > can trivially be worked around with the noexec=off boot option).
>
> We need to be sure because the last thing you want is even 1% of your 10%
> of users to sudden get magical random memory corruption from overriding
> something wrongly !
>
> It's definitely worth checking and worth doing because even if there is
> one (and I have no idea if there is) it'll tell you which steppings
I will report if I find anything bad.
-Kees
--
Kees Cook
Ubuntu Security Team
Hi Alan,
On Tue, Nov 09, 2010 at 04:43:47PM -0800, Kees Cook wrote:
> I looks like there is nothing that actually lists errata directly, so
> walking the entire site for lists of all processor types, e.g.:
> http://www.intel.com/products/processors/previousgeneration/index.htm
> http://www.intel.com/design/celeron/documentation.htm
> and then checking spec updates is the only way to go.
>
> I will report if I find anything bad.
I have reviewed all the spec updates for all the processors I could find
(for a total of 45 PDFs). The only mention of XD is for the Dual-Core Xeon
5100[1]:
AG29. #GP Fault is Not Generated on Writing IA32_MISC_ENABLE [34]
When Execute Disable (XD) is Not Supported
Problem: A #GP fault is not generated on writing to IA32_MISC_ENABLE [34]
bit in a processor which does not support Execute Disable (XD)
functionality.
Implication: Writing to IA32_MISC_ENABLE [34] bit is silently ignored
without generating a fault.
Workaround: None identified.
Status: For the steppings affected, see the Summary Tables of Changes.
But this case is already handled (and doesn't matter) because
my patch already avoids this (we only clear IA32_MISC_ENABLE, not set it).
So, based on that, there doesn't seem to be an errata related to _needing_
to disable the XD cpu feature.
-Kees
[1] ftp://download.intel.com/design/Xeon/specupdt/313356.pdf
--
Kees Cook
Ubuntu Security Team
> So, based on that, there doesn't seem to be an errata related to _needing_
> to disable the XD cpu feature.
Then you have my Ack at least for it.
Alan
* Alan Cox <[email protected]> wrote:
> > So, based on that, there doesn't seem to be an errata related to _needing_
> > to disable the XD cpu feature.
>
> Then you have my Ack at least for it.
Great! Kees, mind posting the latest version of the series with all the Acks
included as well?
Thanks,
Ingo
Kees Cook <[email protected]> writes:
> +
> +verify_cpu_clear_xd:
> + movl $MSR_IA32_MISC_ENABLE, %ecx
> + rdmsr
> + btrl $2, %edx # clear MSR_IA32_MISC_ENABLE_XD_DISABLE
> + jnc verify_cpu_check # only write MSR if bit was
> changed
Strictly it's still a bit dangerous to read this MSR without knowing
about the CPU for sure. If you guess wrong you'll die here.
I would rather move this code later into the early init code (before the
second mapping is set up, which is still in time). There the exception
handlers are up and you could handle a #GP if it happens.
-Andi
--
[email protected] -- Speaking for myself only.
Hi Andi,
On Wed, Nov 10, 2010 at 05:11:55PM +0100, Andi Kleen wrote:
> Kees Cook <[email protected]> writes:
> > +
> > +verify_cpu_clear_xd:
> > + movl $MSR_IA32_MISC_ENABLE, %ecx
> > + rdmsr
> > + btrl $2, %edx # clear MSR_IA32_MISC_ENABLE_XD_DISABLE
> > + jnc verify_cpu_check # only write MSR if bit was
> > changed
>
> Strictly it's still a bit dangerous to read this MSR without knowing
> about the CPU for sure. If you guess wrong you'll die here.
Right, which is why in this code it validates the CPU brand and its
family and model to make sure it's safe to read this MSR. The logic
is identical to the code in early_init_intel() that also accesses
MSR_IA32_MISC_ENABLE. I reviewed the CPU documentation and it seemed
to support that MSR_IA32_MISC_ENABLE would be available under those
conditions. That said, I only had a limited number of systems available
to test it on. If there are adjustments to be made, we can fix them.
> I would rather move this code later into the early init code (before the
> second mapping is set up, which is still in time). There the exception
> handlers are up and you could handle a #GP if it happens.
The problem is that the page tables are set up before early_init, and Peter
Anvin and I did not see a way to move the XD_DISABLE logic any later than
where I've put it. Though I should let Peter speak for himself here, as I'm
less familiar with that aspect of the code.
-Kees
--
Kees Cook
Ubuntu Security Team
> Right, which is why in this code it validates the CPU brand and its
> family and model to make sure it's safe to read this MSR. The logic
> is identical to the code in early_init_intel() that also accesses
> MSR_IA32_MISC_ENABLE. I reviewed the CPU documentation and it seemed
> to support that MSR_IA32_MISC_ENABLE would be available under those
> conditions. That said, I only had a limited number of systems available
> to test it on. If there are adjustments to be made, we can fix them.
If you can even find out. The system will die silently.
Usually the main culprits are simulators. VMs etc. Most do ignore unknown
MSRs, but not all.
>
> > I would rather move this code later into the early init code (before the
> > second mapping is set up, which is still in time). There the exception
> > handlers are up and you could handle a #GP if it happens.
>
> The problem is that the page tables are set up before early_init, and Peter
> Anvin and I did not see a way to move the XD_DISABLE logic any later than
> where I've put it. Though I should let Peter speak for himself here, as I'm
> less familiar with that aspect of the code.
On x86-64 there are two kernel mappings: an early one and a final one.
The final one is only set up in C code.
On 32bit there's only a single one, but it could be changed (e.g.
only add NX later)
-Andi
--
[email protected] -- Speaking for myself only.
On Wed, Nov 10, 2010 at 06:42:10PM +0100, Andi Kleen wrote:
> > Right, which is why in this code it validates the CPU brand and its
> > family and model to make sure it's safe to read this MSR. The logic
> > is identical to the code in early_init_intel() that also accesses
> > MSR_IA32_MISC_ENABLE. I reviewed the CPU documentation and it seemed
> > to support that MSR_IA32_MISC_ENABLE would be available under those
> > conditions. That said, I only had a limited number of systems available
> > to test it on. If there are adjustments to be made, we can fix them.
>
> If you can even find out. The system will die silently.
>
> Usually the main culprits are simulators. VMs etc. Most do ignore unknown
> MSRs, but not all.
Well, that would be a bug in the simulator. :) Those bugs shouldn't cause a
non-trivial group of Linux users to lack NX.
> > > I would rather move this code later into the early init code (before the
> > > second mapping is set up, which is still in time). There the exception
> > > handlers are up and you could handle a #GP if it happens.
> >
> > The problem is that the page tables are set up before early_init, and Peter
> > Anvin and I did not see a way to move the XD_DISABLE logic any later than
> > where I've put it. Though I should let Peter speak for himself here, as I'm
> > less familiar with that aspect of the code.
>
> On x86-64 there are two kernel mappings: an early one and a final one.
> The final one is only set up in C code.
>
> On 32bit there's only a single one, but it could be changed (e.g.
> only add NX later)
I'm open to reviewing tested alternative patches. But right now, this
method is tested and works, and is based on several rounds of design
compromises/refinements.
-Kees
--
Kees Cook
Ubuntu Security Team
On Tue, Nov 09, 2010 at 05:10:39PM -0800, Kees Cook wrote:
> AG29. #GP Fault is Not Generated on Writing IA32_MISC_ENABLE [34]
> When Execute Disable (XD) is Not Supported
> Problem: A #GP fault is not generated on writing to IA32_MISC_ENABLE [34]
> bit in a processor which does not support Execute Disable (XD)
> functionality.
> Implication: Writing to IA32_MISC_ENABLE [34] bit is silently ignored
> without generating a fault.
> Workaround: None identified.
> Status: For the steppings affected, see the Summary Tables of Changes.
> But this case is already handled (and doesn't matter) because my
> patch already avoids this (we only clear IA32_MISC_ENABLE, not set
> it).
Ehhh. A Virtual machine monitor may need to be notified of your
"clearing" that bit. The erratum is correctly worded as for "writing"
not "setting".
But that bug is intel's problem. Not yours.
Roger.
--
** [email protected] ** http://www.BitWizard.nl/ ** +31-15-2600998 **
** Delftechpark 26 2628 XH Delft, The Netherlands. KVK: 27239233 **
*-- BitWizard writes Linux device drivers for any device you may have! --*
Q: It doesn't work. A: Look buddy, doesn't work is an ambiguous statement.
Does it sit on the couch all day? Is it unemployed? Please be specific!
Define 'it' and what it isn't doing. --------- Adapted from lxrbot FAQ