[PATCH] [x86] detect and report lack of NX protections

[PATCH] [x86] detect and report lack of NX protections

[Kees Cook]

It is possible for x86_64 systems to lack the NX bit (see check_efer())
either due to the hardware lacking support or the BIOS having turned
off the CPU capability, so NX status should be reported.  Additionally,
anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
functionality, so this change provides feedback for that case as well.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 arch/x86/mm/init.c     |   10 ++++++++++
 arch/x86/mm/setup_nx.c |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 73ffd55..8472293 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -149,6 +149,16 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	set_nx();
 	if (nx_enabled)
 		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
+	else if (cpu_has_pae)
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+		/* PAE kernel, PAE CPU, without NX */
+		printk(KERN_WARNING "Warning: NX (Execute Disable) protection "
+		       "missing in CPU or disabled in BIOS!\n");
+#else
+		/* 32bit non-PAE kernel, PAE CPU */
+		printk(KERN_WARNING "Warning: NX (Execute Disable) protection "
+		       "cannot be enabled: non-PAE kernel!\n");
+#endif
 
 	/* Enable PSE if available */
 	if (cpu_has_pse)
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index 513d8ed..b039a4c 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -53,6 +53,8 @@ void __init set_nx(void)
 #else
 void set_nx(void)
 {
+	/* notice if _PAGE_NX was removed during check_efer() */
+	nx_enabled = ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX);
 }
 #endif
 
-- 
1.6.3.3


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH] [x86] detect and report lack of NX protections

[Arjan van de Ven]

On Mon, 19 Oct 2009 11:42:34 -0700
Kees Cook <kees.cook@canonical.com> wrote:

> It is possible for x86_64 systems to lack the NX bit (see
> check_efer()) either due to the hardware lacking support or the BIOS
> having turned off the CPU capability, so NX status should be
> reported.  Additionally, anyone booting NX-capable CPUs in 32bit mode
> without PAE will lack NX functionality, so this change provides
> feedback for that case as well.
> 
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
> ---
>  arch/x86/mm/init.c     |   10 ++++++++++
>  arch/x86/mm/setup_nx.c |    2 ++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index 73ffd55..8472293 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -149,6 +149,16 @@ unsigned long __init_refok
> init_memory_mapping(unsigned long start, set_nx();
>  	if (nx_enabled)
>  		printk(KERN_INFO "NX (Execute Disable) protection:
> active\n");
> +	else if (cpu_has_pae)
> +#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
> +		/* PAE kernel, PAE CPU, without NX */
> +		printk(KERN_WARNING "Warning: NX (Execute Disable)
> protection "
> +		       "missing in CPU or disabled in BIOS!\n");
> +#else
> +		/* 32bit non-PAE kernel, PAE CPU */
> +		printk(KERN_WARNING "Warning: NX (Execute Disable)
> protection "
> +		       "cannot be enabled: non-PAE kernel!\n");
> +#endif

can we please not use "Warning" for something like this in the
message...

lets keep "Warning" for real WARN_ON() kind of events.

Re: [PATCH v2] [x86] detect and report lack of NX protections

[Kees Cook]

It is possible for x86_64 systems to lack the NX bit (see check_efer())
either due to the hardware lacking support or the BIOS having turned
off the CPU capability, so NX status should be reported.  Additionally,
anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
functionality, so this change provides feedback for that case as well.

v2: use "Alert:" instead of "Warning:" to avoid confusiong with WARN_ON()

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 arch/x86/mm/init.c     |   10 ++++++++++
 arch/x86/mm/setup_nx.c |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 73ffd55..8472293 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -149,6 +149,16 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	set_nx();
 	if (nx_enabled)
 		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
+	else if (cpu_has_pae)
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+		/* PAE kernel, PAE CPU, without NX */
+		printk(KERN_WARNING "Alert: NX (Execute Disable) protection "
+		       "missing in CPU or disabled in BIOS!\n");
+#else
+		/* 32bit non-PAE kernel, PAE CPU */
+		printk(KERN_WARNING "Alert: NX (Execute Disable) protection "
+		       "cannot be enabled: non-PAE kernel!\n");
+#endif
 
 	/* Enable PSE if available */
 	if (cpu_has_pse)
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index 513d8ed..b039a4c 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -53,6 +53,8 @@ void __init set_nx(void)
 #else
 void set_nx(void)
 {
+	/* notice if _PAGE_NX was removed during check_efer() */
+	nx_enabled = ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX);
 }
 #endif
 
-- 
1.6.3.3


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v2] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 10/20/2009 11:04 AM, Kees Cook wrote:
> It is possible for x86_64 systems to lack the NX bit (see check_efer())
> either due to the hardware lacking support or the BIOS having turned
> off the CPU capability, so NX status should be reported.  Additionally,
> anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
> functionality, so this change provides feedback for that case as well.
>
> v2: use "Alert:" instead of "Warning:" to avoid confusiong with WARN_ON()
>

They're both wrong.  Both imply that the user needs to take an action, 
which is wrong because the kernel is working as intended.  If you need 
to use any kind of alert word, it should be something like "Notice:", 
and it should be KERN_NOTICE or even KERN_INFO.

	-hpa

Re: [PATCH v2] [x86] detect and report lack of NX protections

[Kees Cook]

Hi,

On Tue, Oct 20, 2009 at 11:18:43AM +0900, H. Peter Anvin wrote:
> On 10/20/2009 11:04 AM, Kees Cook wrote:
> >It is possible for x86_64 systems to lack the NX bit (see check_efer())
> >either due to the hardware lacking support or the BIOS having turned
> >off the CPU capability, so NX status should be reported.  Additionally,
> >anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
> >functionality, so this change provides feedback for that case as well.
> >
> >v2: use "Alert:" instead of "Warning:" to avoid confusiong with WARN_ON()
> >
> 
> They're both wrong.  Both imply that the user needs to take an
> action, which is wrong because the kernel is working as intended.
> If you need to use any kind of alert word, it should be something
> like "Notice:", and it should be KERN_NOTICE or even KERN_INFO.

In the case of a system where the BIOS was shipped with XD not enabled,
the user needs to take an action.  I'm okay with switching to Notice:, but
I don't think KERN_INFO is right.  I would agree, "Alert:" would seem to
be a KERN_ALERT, which is above KERN_CRIT, which this is clearly not.
"Notice" it is.

-Kees

-- 
Kees Cook
Ubuntu Security Team

[PATCH v3] [x86] detect and report lack of NX protections

[Kees Cook]

It is possible for x86_64 systems to lack the NX bit (see check_efer())
either due to the hardware lacking support or the BIOS having turned
off the CPU capability, so NX status should be reported.  Additionally,
anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
functionality, so this change provides feedback for that case as well.

v2: use "Alert:" instead of "Warning:" to avoid confusion with WARN_ON()
v3: use "Notice:" instead of "Alert:" to avoid confusion with KERN_ALERT,
    and switch to KERN_NOTICE, in keeping with its use for "normal but
    significant condition" messages.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 arch/x86/mm/init.c     |   10 ++++++++++
 arch/x86/mm/setup_nx.c |    2 ++
 2 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 73ffd55..8472293 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -149,6 +149,16 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	set_nx();
 	if (nx_enabled)
 		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
+	else if (cpu_has_pae)
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+		/* PAE kernel, PAE CPU, without NX */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "missing in CPU or disabled in BIOS!\n");
+#else
+		/* 32bit non-PAE kernel, PAE CPU */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "cannot be enabled: non-PAE kernel!\n");
+#endif
 
 	/* Enable PSE if available */
 	if (cpu_has_pse)
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index 513d8ed..b039a4c 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -53,6 +53,8 @@ void __init set_nx(void)
 #else
 void set_nx(void)
 {
+	/* notice if _PAGE_NX was removed during check_efer() */
+	nx_enabled = ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX);
 }
 #endif
 
-- 
1.6.3.3


-- 
Kees Cook
Ubuntu Security Team

[PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

It is possible for x86_64 systems to lack the NX bit (see check_efer())
either due to the hardware lacking support or the BIOS having turned
off the CPU capability, so NX status should be reported.  Additionally,
anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
functionality, so this change provides feedback for that case as well.

v2: use "Alert:" instead of "Warning:" to avoid confusion with WARN_ON()
v3: use "Notice:" instead of "Alert:" to avoid confusion with KERN_ALERT,
    and switch to KERN_NOTICE, in keeping with its use for "normal but
    significant condition" messages.
v4: check that _NX_PAGE is non-zero to avoid setting nx_enabled accidentally.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 arch/x86/mm/init.c     |   10 ++++++++++
 arch/x86/mm/setup_nx.c |    3 +++
 2 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 73ffd55..d98b43a 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -149,6 +149,16 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	set_nx();
 	if (nx_enabled)
 		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
+	else if (cpu_has_pae)
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+		/* PAE kernel, PAE CPU, without NX */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "missing in CPU or disabled in BIOS!\n");
+#else
+		/* 32bit non-PAE kernel, PAE CPU */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "cannot be enabled: non-PAE kernel!\n");
+#endif
 
 	/* Enable PSE if available */
 	if (cpu_has_pse)
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index 513d8ed..1b93231 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -53,6 +53,9 @@ void __init set_nx(void)
 #else
 void set_nx(void)
 {
+	/* notice if _PAGE_NX exists and was removed during check_efer() */
+	if (_PAGE_NX && ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX))
+		nx_enabled = 1;
 }
 #endif
 
-- 
1.6.5


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/09/2009 02:10 PM, Kees Cook wrote:
> It is possible for x86_64 systems to lack the NX bit (see check_efer())
> either due to the hardware lacking support or the BIOS having turned
> off the CPU capability, so NX status should be reported.  Additionally,
> anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
> functionality, so this change provides feedback for that case as well.
> 
> v2: use "Alert:" instead of "Warning:" to avoid confusion with WARN_ON()
> v3: use "Notice:" instead of "Alert:" to avoid confusion with KERN_ALERT,
>     and switch to KERN_NOTICE, in keeping with its use for "normal but
>     significant condition" messages.
> v4: check that _NX_PAGE is non-zero to avoid setting nx_enabled accidentally.
> 
> Signed-off-by: Kees Cook <kees.cook@canonical.com>
> ---
>  arch/x86/mm/init.c     |   10 ++++++++++
>  arch/x86/mm/setup_nx.c |    3 +++
>  2 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index 73ffd55..d98b43a 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -149,6 +149,16 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
>  	set_nx();
>  	if (nx_enabled)
>  		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
> +	else if (cpu_has_pae)
> +#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
> +		/* PAE kernel, PAE CPU, without NX */
> +		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
> +		       "missing in CPU or disabled in BIOS!\n");
> +#else
> +		/* 32bit non-PAE kernel, PAE CPU */
> +		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
> +		       "cannot be enabled: non-PAE kernel!\n");
> +#endif
>  
>  	/* Enable PSE if available */
>  	if (cpu_has_pse)
> diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
> index 513d8ed..1b93231 100644
> --- a/arch/x86/mm/setup_nx.c
> +++ b/arch/x86/mm/setup_nx.c
> @@ -53,6 +53,9 @@ void __init set_nx(void)
>  #else
>  void set_nx(void)
>  {
> +	/* notice if _PAGE_NX exists and was removed during check_efer() */
> +	if (_PAGE_NX && ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX))
> +		nx_enabled = 1;
>  }
>  #endif
>  

The second clause can only get executed if CONFIG_X86_PAE is unset,
which in turn means _PAGE_NX == 0... so that piece of code is meaningless.

It also looks to me that there is no message distinguishing the case
when nx_enabled == 1 but disable_nx == 1, and instead we say NX is
"active" when in fact it is disabled in the kernel.

	-hpa

Re: [PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

On Mon, Nov 09, 2009 at 03:16:16PM -0800, H. Peter Anvin wrote:
> On 11/09/2009 02:10 PM, Kees Cook wrote:
> > diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
> > index 513d8ed..1b93231 100644
> > --- a/arch/x86/mm/setup_nx.c
> > +++ b/arch/x86/mm/setup_nx.c
> > @@ -53,6 +53,9 @@ void __init set_nx(void)
> >  #else
> >  void set_nx(void)
> >  {
> > +	/* notice if _PAGE_NX exists and was removed during check_efer() */
> > +	if (_PAGE_NX && ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX))
> > +		nx_enabled = 1;
> >  }
> >  #endif
> >  
> 
> The second clause can only get executed if CONFIG_X86_PAE is unset,
> which in turn means _PAGE_NX == 0... so that piece of code is meaningless.

CONFIG_X86_PAE is unset for x86_64, where _PAGE_NX is valid.  (This was
the main situation I was trying to address.)  So that chunk runs for
non-pae 32bit, and all 64bit:

config X86_PAE
        bool "PAE (Physical Address Extension) Support"
        depends on X86_32 && !HIGHMEM4G

> It also looks to me that there is no message distinguishing the case
> when nx_enabled == 1 but disable_nx == 1, and instead we say NX is
> "active" when in fact it is disabled in the kernel.

That's true -- I had overlooked that part.  New patch on the way...

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/10/2009 07:49 AM, Kees Cook wrote:
>>
>> The second clause can only get executed if CONFIG_X86_PAE is unset,
>> which in turn means _PAGE_NX == 0... so that piece of code is meaningless.
> 
> CONFIG_X86_PAE is unset for x86_64, where _PAGE_NX is valid.  (This was
> the main situation I was trying to address.)  So that chunk runs for
> non-pae 32bit, and all 64bit:
> 

In reality, X86_PAE really should have been defined for 64 bits, since
64 bits really is PAE in most aspects.

Anyway, for the 64-bit case it looks like the proper place for any of
this is in check_efer() just below, not in this null routine.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

[PATCH v5] [x86] detect and report lack of NX protections

[Kees Cook]

It is possible for x86_64 systems to lack the NX bit (see check_efer())
either due to the hardware lacking support or the BIOS having turned
off the CPU capability, so NX status should be reported.  Additionally,
anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
functionality, so this change provides feedback for that case as well.

v2: use "Alert:" instead of "Warning:" to avoid confusion with WARN_ON()
v3: use "Notice:" instead of "Alert:" to avoid confusion with KERN_ALERT,
    and switch to KERN_NOTICE, in keeping with its use for "normal but
    significant condition" messages.
v4: check that _NX_PAGE is non-zero to avoid setting nx_enabled accidentally.
v5: check for disable_nx.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 arch/x86/mm/init.c     |   10 ++++++++++
 arch/x86/mm/setup_nx.c |    6 +++++-
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 73ffd55..d98b43a 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -149,6 +149,16 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	set_nx();
 	if (nx_enabled)
 		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
+	else if (cpu_has_pae)
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+		/* PAE kernel, PAE CPU, without NX */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "missing in CPU or disabled in BIOS!\n");
+#else
+		/* 32bit non-PAE kernel, PAE CPU */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "cannot be enabled: non-PAE kernel!\n");
+#endif
 
 	/* Enable PSE if available */
 	if (cpu_has_pse)
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index 513d8ed..93d7b43 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -51,8 +51,12 @@ void __init set_nx(void)
 	}
 }
 #else
-void set_nx(void)
+void __init set_nx(void)
 {
+	/* notice if _PAGE_NX exists and was removed during check_efer() */
+	if (!disable_nx && _PAGE_NX &&
+	    ((__supported_pte_mask & _PAGE_NX) == _PAGE_NX))
+		nx_enabled = 1;
 }
 #endif
 
-- 
1.6.5

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

On Tue, Nov 10, 2009 at 08:47:23AM -0800, H. Peter Anvin wrote:
> On 11/10/2009 07:49 AM, Kees Cook wrote:
> >>
> >> The second clause can only get executed if CONFIG_X86_PAE is unset,
> >> which in turn means _PAGE_NX == 0... so that piece of code is meaningless.
> > 
> > CONFIG_X86_PAE is unset for x86_64, where _PAGE_NX is valid.  (This was
> > the main situation I was trying to address.)  So that chunk runs for
> > non-pae 32bit, and all 64bit:
> > 
> 
> In reality, X86_PAE really should have been defined for 64 bits, since
> 64 bits really is PAE in most aspects.
> 
> Anyway, for the 64-bit case it looks like the proper place for any of
> this is in check_efer() just below, not in this null routine.

64-bit does not set nx_enabled to "1" by default anywhere.  And setting
the default to 1 in check_efer() seemed out of place to me.

-Kees

-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/10/2009 08:57 AM, Kees Cook wrote:
> 
> 64-bit does not set nx_enabled to "1" by default anywhere.  And setting
> the default to 1 in check_efer() seemed out of place to me.
> 

If it doesn't set nx_enabled anywhere, you'll end up with the message

+	printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+	       "missing in CPU or disabled in BIOS!\n");

which seems obviously wrong... no?

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

On Tue, Nov 10, 2009 at 09:12:44AM -0800, H. Peter Anvin wrote:
> On 11/10/2009 08:57 AM, Kees Cook wrote:
> > 
> > 64-bit does not set nx_enabled to "1" by default anywhere.  And setting
> > the default to 1 in check_efer() seemed out of place to me.
> > 
> 
> If it doesn't set nx_enabled anywhere, you'll end up with the message
> 
> +	printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
> +	       "missing in CPU or disabled in BIOS!\n");
> 
> which seems obviously wrong... no?

The kernel as-is does not set nx_enabled for 64-bit, so this message is
skipped completely:

        if (nx_enabled)
                printk(KERN_INFO "NX (Execute Disable) protection: active\n");

The only time this printk is shown is on 32-bit with PAE (with NX).
There is no "else" currently.


My patch seeks to do two things:
 - report the "active" message on 64-bit (if not disabled in check_efer())
 - report "zomg NX missing" for all cases when PAE is available to the CPU.

To implement the first, the set_nx() routine for non-PAE 32-bit and all
64-bit needs to have nx_enabled set correctly (which is done in the second
set_nx() that was empty in arch/x86/mm/setup_nx.c).

To implement the second, the following logic is then added to the "if
(nx_enabled)" case in arch/x86/mm/init.c as an "else":

        else if (cpu_has_pae)
#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
                /* PAE kernel, PAE CPU, without NX */
                printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
                       "missing in CPU or disabled in BIOS!\n");
#else
                /* 32bit non-PAE kernel, PAE CPU */
                printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
                       "cannot be enabled: non-PAE kernel!\n");
#endif


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/10/2009 09:46 AM, Kees Cook wrote:
> 
> The kernel as-is does not set nx_enabled for 64-bit, so this message is
> skipped completely:
> 
>         if (nx_enabled)
>                 printk(KERN_INFO "NX (Execute Disable) protection: active\n");
> 
> The only time this printk is shown is on 32-bit with PAE (with NX).
> There is no "else" currently.
> 

The structure you have is:

	if (nx_enabled)
	else if (cpu_has_pae)

The test for cpu_has_pae is unconditional (you only #ifdef the message)
-- in fact, this should cause a compile-time error on 64 bits:

#undef  cpu_has_pae
#define cpu_has_pae             ___BUG___

	-hpa

Re: [PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

On Tue, Nov 10, 2009 at 10:53:33AM -0800, H. Peter Anvin wrote:
> On 11/10/2009 09:46 AM, Kees Cook wrote:
> > 
> > The kernel as-is does not set nx_enabled for 64-bit, so this message is
> > skipped completely:
> > 
> >         if (nx_enabled)
> >                 printk(KERN_INFO "NX (Execute Disable) protection: active\n");
> > 
> > The only time this printk is shown is on 32-bit with PAE (with NX).
> > There is no "else" currently.
> > 
> 
> The structure you have is:
> 
> 	if (nx_enabled)
> 	else if (cpu_has_pae)
> 
> The test for cpu_has_pae is unconditional (you only #ifdef the message)
> -- in fact, this should cause a compile-time error on 64 bits:
> 
> #undef  cpu_has_pae
> #define cpu_has_pae             ___BUG___

This is fun.  CONFIG_X86_PAE isn't defined for 64-bit, and using
cpu_has_pae on 64-bit is considered a bug.  :)

Here is the matrix of what I want to see reported about NX at boot time.
How do you recommend this be implemented?

kernel  cpu -> |              CPU has PAE              |  CPU lacks PAE  |
   |           |       CPU has NX  | CPU lacks NX      |                 |
   V           +-------------------+-------------------+-----------------+
32-bit non-PAE | missing in kernel | missing in kernel |    no message   |
               +-------------------+-------------------+-----------------+
32-bit PAE     | active *          | missing in CPU    |    no message   |
               +-------------------+-------------------+-----------------+
64-bit         | active            | missing in CPU    |    impossible   |
               +-------------------+-------------------+-----------------+
The box with the "*" is the only message currently reported by the kernel.


this looks ugly, but does it:

#if defined(CONFIG_X86_32)
	else if (cpu_has_pae)
#else
	else
#endif
#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
		/* PAE kernel, PAE CPU, without NX */
		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
		       "missing in CPU or disabled in BIOS!\n");
#else
		/* 32bit non-PAE kernel, PAE CPU */
		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
		       "cannot be enabled: non-PAE kernel!\n");
#endif


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/10/2009 11:43 AM, Kees Cook wrote:
> 
> This is fun.  CONFIG_X86_PAE isn't defined for 64-bit, and using
> cpu_has_pae on 64-bit is considered a bug.  :)
> 

Yeah, it's somewhat obnoxious.  This stuff is a result of the 32- and
64-bit code evolving separately for too long.  All of this could and
should be cleaned up, but it takes a long time.

Either way, you can use the explicit form:

	boot_cpu_has(X86_FEATURE_PAE)

just fine, on any platform.  However, the only case for which this can
be false is for the non-PAE kernel, since the PAE kernels (32 or 64
bits) cannot boot without it.  I have personally never liked the
cpu_has_* shorthand macros, but they're occasionally useful for things
that have to be handled specially on 64 bits.  Unfortunately they have
spread and people seem to think they're the only way.

> Here is the matrix of what I want to see reported about NX at boot time.
> How do you recommend this be implemented?
> 
> kernel  cpu -> |              CPU has PAE              |  CPU lacks PAE  |
>    |           |       CPU has NX  | CPU lacks NX      |                 |
>    V           +-------------------+-------------------+-----------------+
> 32-bit non-PAE | missing in kernel | missing in kernel |    no message   |
>                +-------------------+-------------------+-----------------+
> 32-bit PAE     | active *          | missing in CPU    |    no message   |
>                +-------------------+-------------------+-----------------+
> 64-bit         | active            | missing in CPU    |    impossible   |
>                +-------------------+-------------------+-----------------+
> The box with the "*" is the only message currently reported by the kernel.

The last column should actually be "no message", "impossible", "impossible".

I also think "missing in kernel" is misleading in the 32-bit non-PAE,
no-NX case (as it would imply that another kernel could do something),
and I *really* fail to see why it is in any way different from the "CPU
lacks PAE" case -- which also means no NX.  "Unavailable in CPU" seems
to beat everything.

So the logic that makes sense would be:

if (!cpu_has_nx) {
	/* If the CPU can't do it... */
	printk(KERN_INFO "cpu: NX protection unavailable in CPU\n");
} else  {
#if defined(CONFIG_X86_32) && !defined(CONFIG_X86_PAE)
	/* Non-PAE kernel: NX unavailable */
	printk(KERN_NOTICE "cpu: NX protection missing in kernel\n");
#else
	printk(KERN_INFO "cpu: NX protection active\n");
#endif
}

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

The more I stare at the underlying code, the more I'm convinced that the
fundamental problem is that the underlying code is insane, with multiple
levels of detection for what amounts to cpu_has_nx, each effectively
checking what the previous code has done.

check_efer(), for example, screws with EFER, but EFER is simply set in
head_64.S from CPUID (unless Xen does something insane -- but if so, Xen
should clear X86_FEATURE_NX instead.)

The 32-bit startup code also sets NX, but yet on 32 bits we wiggle EFER
as if it had never been.  This code is screaming for cleanup and
unification.

	-hpa

Re: [PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

On Tue, Nov 10, 2009 at 11:59:35AM -0800, H. Peter Anvin wrote:
> On 11/10/2009 11:43 AM, Kees Cook wrote:
> > kernel  cpu -> |              CPU has PAE              |  CPU lacks PAE  |
> >    |           |       CPU has NX  | CPU lacks NX      |                 |
> >    V           +-------------------+-------------------+-----------------+
> > 32-bit non-PAE | missing in kernel | missing in kernel |    no message   |
> >                +-------------------+-------------------+-----------------+
> > 32-bit PAE     | active *          | missing in CPU    |    no message   |
> >                +-------------------+-------------------+-----------------+
> > 64-bit         | active            | missing in CPU    |    impossible   |
> >                +-------------------+-------------------+-----------------+
> > The box with the "*" is the only message currently reported by the kernel.
> 
> The last column should actually be "no message", "impossible", "impossible".

Ah, yes, very true.

> I also think "missing in kernel" is misleading in the 32-bit non-PAE,
> no-NX case (as it would imply that another kernel could do something),

Well, I think thinking that even if they turned on the flag in the BIOS,
the non-PAE kernel couldn't do anything about it anyway.  But, from your
example, I see you went with "missing in kernel" anyway.

> and I *really* fail to see why it is in any way different from the "CPU
> lacks PAE" case -- which also means no NX.  "Unavailable in CPU" seems
> to beat everything.

I was trying to be conservative and not have older hardware without PAE
yelling about missing features.  It seemed only worth complaining about
missing NX if PAE was possible (the class of machines without PAE being
larger than those without NX and with PAE).

> So the logic that makes sense would be:
> 
> if (!cpu_has_nx) {

cpu_has_nx is not the same as nx_enabled (due to disable_nx).  Also, why
doesn't set_nx() use cpu_has_nx?  It seems like it does the check
manually?  Should that be cleaned up?

> 	/* If the CPU can't do it... */
> 	printk(KERN_INFO "cpu: NX protection unavailable in CPU\n");
> } else  {
> #if defined(CONFIG_X86_32) && !defined(CONFIG_X86_PAE)
> 	/* Non-PAE kernel: NX unavailable */
> 	printk(KERN_NOTICE "cpu: NX protection missing in kernel\n");
> #else
> 	printk(KERN_INFO "cpu: NX protection active\n");
> #endif
> }

How about this?  (Along with the nx_enabled setting in set_nx() for the
64-bit and 32-bit+PAE case.)

        set_nx();
        if (nx_enabled) {
                printk(KERN_INFO "NX (Execute Disable) protection: active\n");
#if defined(CONFIG_X86_32) && !defined(CONFIG_X86_PAE)
        } else if (cpu_has_pae) {
                /* 32bit non-PAE kernel, PAE CPU */
                printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
                       "cannot be enabled: non-PAE kernel!\n");
#else
        } else {
                /* PAE kernel, PAE CPU, without NX */
                printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
                       "missing in CPU or disabled in BIOS!\n");
#endif
        }


-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/10/2009 12:55 PM, Kees Cook wrote:
>> > I also think "missing in kernel" is misleading in the 32-bit non-PAE,
>> > no-NX case (as it would imply that another kernel could do something),
> Well, I think thinking that even if they turned on the flag in the BIOS,
> the non-PAE kernel couldn't do anything about it anyway.  But, from your
> example, I see you went with "missing in kernel" anyway.

No, I didn't: in my example, the CPU checks have higher priority than
the kernel feature check.

>> So the logic that makes sense would be:
>>
>> if (!cpu_has_nx) {
> 
> cpu_has_nx is not the same as nx_enabled (due to disable_nx).  Also, why
> doesn't set_nx() use cpu_has_nx?  It seems like it does the check
> manually?  Should that be cleaned up?

Yes, it should be.  set_nx() and check_efer() are doing the same thing,
except in different ways, and they are - IMO - *both* doing something
dumb -- although check_efer() is saner.

Anyway, I forgot the last case, which is NX disabled manually
(disable_nx).  It probably makes sense to make it the lowest priority
message.

if (!cpu_has_nx) {
	/* If the CPU can't do it... */
	printk(KERN_INFO "cpu: NX protection unavailable in CPU\n");
} else  {
#if defined(CONFIG_X86_32) && !defined(CONFIG_X86_PAE)
	/* Non-PAE kernel: NX unavailable */
	printk(KERN_NOTICE "cpu: NX protection not supported by kernel\n");
#else
	if (disable_nx)
		printk(KERN_INFO "cpu: NX protection disabled by kernel command line
option\n");
	else
		printk(KERN_INFO "cpu: NX protection active\n");
#endif
}


> How about this?  (Along with the nx_enabled setting in set_nx() for the
> 64-bit and 32-bit+PAE case.)

No, it gives the wrong message for the manually disabled case.

	-hpa

Re: [PATCH v4] [x86] detect and report lack of NX protections

[Kees Cook]

On Tue, Nov 10, 2009 at 01:22:18PM -0800, H. Peter Anvin wrote:
> Yes, it should be.  set_nx() and check_efer() are doing the same thing,
> except in different ways, and they are - IMO - *both* doing something
> dumb -- although check_efer() is saner.

BTW, it seems like set_nx() should move from init_memory_mapping to
setup_arch, since init_memory_mapping can be called twice, but I don't
think set_nx needs to be.

Also, disable_nx is only defined in setup_nx.c when
"defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)", so I re-arranged the
logic to match that.

How about the following clean-up and merge...

---
It is possible for x86_64 systems to lack the NX bit (see check_efer())
either due to the hardware lacking support or the BIOS having turned
off the CPU capability, so NX status should be reported.  Additionally,
anyone booting NX-capable CPUs in 32bit mode without PAE will lack NX
functionality, so this change provides feedback for that case as well.

Signed-off-by: Kees Cook <kees.cook@canonical.com>
---
 arch/x86/kernel/setup.c |    2 ++
 arch/x86/mm/init.c      |    4 ----
 arch/x86/mm/setup_nx.c  |   31 ++++++++++++++++++++-----------
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index e09f0e2..72181c1 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -895,6 +895,8 @@ void __init setup_arch(char **cmdline_p)
 
 	init_gbpages();
 
+	set_nx();
+
 	/* max_pfn_mapped is updated here */
 	max_low_pfn_mapped = init_memory_mapping(0, max_low_pfn<<PAGE_SHIFT);
 	max_pfn_mapped = max_low_pfn_mapped;
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 73ffd55..d406c52 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -146,10 +146,6 @@ unsigned long __init_refok init_memory_mapping(unsigned long start,
 	use_gbpages = direct_gbpages;
 #endif
 
-	set_nx();
-	if (nx_enabled)
-		printk(KERN_INFO "NX (Execute Disable) protection: active\n");
-
 	/* Enable PSE if available */
 	if (cpu_has_pse)
 		set_in_cr4(X86_CR4_PSE);
diff --git a/arch/x86/mm/setup_nx.c b/arch/x86/mm/setup_nx.c
index 513d8ed..269b668 100644
--- a/arch/x86/mm/setup_nx.c
+++ b/arch/x86/mm/setup_nx.c
@@ -33,28 +33,37 @@ static int __init noexec_setup(char *str)
 early_param("noexec", noexec_setup);
 #endif
 
-#ifdef CONFIG_X86_PAE
 void __init set_nx(void)
 {
-	unsigned int v[4], l, h;
-
-	if (cpu_has_pae && (cpuid_eax(0x80000000) > 0x80000001)) {
-		cpuid(0x80000001, &v[0], &v[1], &v[2], &v[3]);
+	if (!cpu_has_nx) {
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "missing in CPU or disabled in BIOS!\n");
+	} else {
+#if defined(CONFIG_X86_64) || defined(CONFIG_X86_PAE)
+		if (disable_nx)
+			printk(KERN_INFO "NX (Execute Disable) protection: "
+			       "disabled by kernel command line option\n");
+		else {
+# ifdef CONFIG_X86_PAE
+			/* 32-bit PAE */
+			unsigned int l, h;
 
-		if ((v[3] & (1 << 20)) && !disable_nx) {
 			rdmsr(MSR_EFER, l, h);
 			l |= EFER_NX;
 			wrmsr(MSR_EFER, l, h);
 			nx_enabled = 1;
 			__supported_pte_mask |= _PAGE_NX;
+# endif
+			printk(KERN_INFO "NX (Execute Disable) protection: "
+			       "active\n");
 		}
-	}
-}
 #else
-void set_nx(void)
-{
-}
+		/* 32bit non-PAE kernel, NX cannot be used */
+		printk(KERN_NOTICE "Notice: NX (Execute Disable) protection "
+		       "cannot be enabled: non-PAE kernel!\n");
 #endif
+	}
+}
 
 #ifdef CONFIG_X86_64
 void __cpuinit check_efer(void)
-- 
1.6.5



-- 
Kees Cook
Ubuntu Security Team

Re: [PATCH v4] [x86] detect and report lack of NX protections

[H. Peter Anvin]

On 11/10/2009 02:15 PM, Kees Cook wrote:
> On Tue, Nov 10, 2009 at 01:22:18PM -0800, H. Peter Anvin wrote:
>> Yes, it should be.  set_nx() and check_efer() are doing the same thing,
>> except in different ways, and they are - IMO - *both* doing something
>> dumb -- although check_efer() is saner.
> 
> BTW, it seems like set_nx() should move from init_memory_mapping to
> setup_arch, since init_memory_mapping can be called twice, but I don't
> think set_nx needs to be.
> 

We already have call points in setup_arch for check_efer(), which are
where this code needs to be called.

As I said, check_efer() and set_nx() are the 64- and 32-bit versions of
*exactly the same thing*.  Furthermore, it looks to me that set_nx() is
simply broken; it forces the bit set in EFER even though it is already
set in head_32.S.

Thus, what we *should* do is:

a) remove set_nx() completely;
b) enable check_efer() on 32 bits;
c) rename check_efer() to something sane, like x86_configure_nx();

Optionally we should just use cpu_has_nx instead of reading EFER.

	-hpa

RE: [PATCH v4] [x86] detect and report lack of NX protections

[Yuhong Bao]

>> The last column should actually be "no message", "impossible", "impossible".
>
> Ah, yes, very true.
Nope, not only can it be disabled in the BIOS, but also don't forget Intel's first D0 stepping Nocona Xeon x86-64 CP\x7fUs, which do not have NX.
> (the class of machines without PAE being
> larger than those without NX and with PAE).Nope also. Clue: Most post-Pentium Pro Intel CPUs and all AMD CPUs since the K7 has PAE available as a CPU feature.
Yuhong Bao 		 	   		  
_________________________________________________________________
Hotmail: Powerful Free email with security by Microsoft.
http://clk.atdmt.com/GBL/go/171222986/direct/01/