[PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[matthieu castet]

[-- Attachment #1: Type: text/plain, Size: 141 bytes --]

I think it should be applied before 2.6.38 release, because without
this patch S3 suspend doesn't work on x86_32 with CONFIG_DEBUG_RODATA.



[-- Attachment #2: 0001-NX-protection-for-kernel-data-fix-32-bits-S3-suspend.patch --]
[-- Type: text/x-diff, Size: 1911 bytes --]

>From a8d56e665c9b26c953f355b6e8eeeecafa07efdb Mon Sep 17 00:00:00 2001
From: Matthieu CASTET <castet.matthieu@free.fr>
Date: Thu, 27 Jan 2011 21:36:07 +0100
Subject: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

32 bits wakeup realmode trampoline enable paging, while still
in low memory.

We should make this memory !NX in order it works.

Signed-off-by: Matthieu CASTET <castet.matthieu@free.fr>
Tested-by: Matthias Hopf <mhopf@suse.de>
---
 arch/x86/mm/init_32.c  |    8 ++++++++
 arch/x86/mm/pageattr.c |    7 +++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index c821074..0048738 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -227,6 +227,14 @@ page_table_range_init(unsigned long start, unsigned long end, pgd_t *pgd_base)
 
 static inline int is_kernel_text(unsigned long addr)
 {
+#if defined(CONFIG_X86_32) && defined(CONFIG_ACPI_SLEEP)
+	/*
+	 * We need to make the wakeup trampoline in first 1MB !NX
+	 */
+	if (addr >= PAGE_OFFSET && addr <= (PAGE_OFFSET + (1<<20)))
+		return 1;
+#endif
+
 	if (addr >= (unsigned long)_text && addr <= (unsigned long)__init_end)
 		return 1;
 	return 0;
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index d343b3c..f1d6cf5 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -265,6 +265,13 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
 	if (pcibios_enabled && within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_NX;
 #endif
+	/*
+	 * We need to make the wakeup trampoline in first 1MB !NX
+	 */
+#if defined(CONFIG_X86_32) && defined(CONFIG_ACPI_SLEEP)
+	if (within(address, PAGE_OFFSET, PAGE_OFFSET + (1<<20)))
+		pgprot_val(forbidden) |= _PAGE_NX;
+#endif
 
 	/*
 	 * The kernel text needs to be executable for obvious reasons
-- 
1.7.2.3

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Ingo Molnar]

* matthieu castet <castet.matthieu@free.fr> wrote:

>  static inline int is_kernel_text(unsigned long addr)
>  {
> +#if defined(CONFIG_X86_32) && defined(CONFIG_ACPI_SLEEP)
> +	/*
> +	 * We need to make the wakeup trampoline in first 1MB !NX
> +	 */
> +	if (addr >= PAGE_OFFSET && addr <= (PAGE_OFFSET + (1<<20)))
> +		return 1;
> +#endif

That's pretty ugly. Why not use set_memory_x()/set_memory_nx(), and only for the 
trampoline itself? Does the whole 1MB need to be marked X?

Same goes for the Xen fix.

Thanks,

	Ingo

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[castet.matthieu]

Quoting Ingo Molnar <mingo@elte.hu>:

>
> * matthieu castet <castet.matthieu@free.fr> wrote:
>
> >  static inline int is_kernel_text(unsigned long addr)
> >  {
> > +#if defined(CONFIG_X86_32) && defined(CONFIG_ACPI_SLEEP)
> > +	/*
> > +	 * We need to make the wakeup trampoline in first 1MB !NX
> > +	 */
> > +	if (addr >= PAGE_OFFSET && addr <= (PAGE_OFFSET + (1<<20)))
> > +		return 1;
> > +#endif
>
> That's pretty ugly. Why not use set_memory_x()/set_memory_nx(), and only for
> the
> trampoline itself? Does the whole 1MB need to be marked X?
The previous code was doing that.

If you prefer I can revert to the old code :

static inline int is_kernel_text(unsigned long addr)
{
    if (addr >= PAGE_OFFSET && addr <= (unsigned long)__init_end)
        return 1;
    return 0;
}

The proper fix will be done in 2.6.29, when S3 resume 32 bit trampoline will be
merged with 64 bit (ie call smp trampoline) [1]

>
> Same goes for the Xen fix.
>
The Xen fix reverts code to old behavior.
We can't assume that all data/bss will be RW. And I see no way to detect witch
page in data/bss we should not force to RW in static protection.
So we assume the code that change protection of data/bss knows what it is doing
(that what was doing old code).

Matthieu


[1]http://marc.info/?l=linux-kernel&m=129616541603610&w=2

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/01/2011 05:25 AM, castet.matthieu@free.fr wrote:
>
> The proper fix will be done in 2.6.29, when S3 resume 32 bit trampoline will be
> merged with 64 bit (ie call smp trampoline) [1]
>

That's insufficient; the actual unified lowmem trampoline allocator 
should take care of that bit per se.

However, the bottom line seems to be that this patch was done with some 
extreme lack of care.  To really be proper, every allocation from the 
get go -- including the brk and memblock based ones -- should be tagged 
with their execution status, and that information should be used, not 
some magic ranges.

	-hpa

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Ingo Molnar]

* castet.matthieu@free.fr <castet.matthieu@free.fr> wrote:

> Quoting Ingo Molnar <mingo@elte.hu>:
> 
> >
> > * matthieu castet <castet.matthieu@free.fr> wrote:
> >
> > >  static inline int is_kernel_text(unsigned long addr)
> > >  {
> > > +#if defined(CONFIG_X86_32) && defined(CONFIG_ACPI_SLEEP)
> > > +	/*
> > > +	 * We need to make the wakeup trampoline in first 1MB !NX
> > > +	 */
> > > +	if (addr >= PAGE_OFFSET && addr <= (PAGE_OFFSET + (1<<20)))
> > > +		return 1;
> > > +#endif
> >
> > That's pretty ugly. Why not use set_memory_x()/set_memory_nx(), and only for
> > the
> > trampoline itself? Does the whole 1MB need to be marked X?
>
> The previous code was doing that.

So why not call set_memory_x() in your patch? Mind trying that?

Thanks,

	Ingo

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/01/2011 10:26 PM, Ingo Molnar wrote:
> 
> * castet.matthieu@free.fr <castet.matthieu@free.fr> wrote:
> 
>> Quoting Ingo Molnar <mingo@elte.hu>:
>>
>>>
>>> * matthieu castet <castet.matthieu@free.fr> wrote:
>>>
>>>>  static inline int is_kernel_text(unsigned long addr)
>>>>  {
>>>> +#if defined(CONFIG_X86_32) && defined(CONFIG_ACPI_SLEEP)
>>>> +	/*
>>>> +	 * We need to make the wakeup trampoline in first 1MB !NX
>>>> +	 */
>>>> +	if (addr >= PAGE_OFFSET && addr <= (PAGE_OFFSET + (1<<20)))
>>>> +		return 1;
>>>> +#endif
>>>
>>> That's pretty ugly. Why not use set_memory_x()/set_memory_nx(), and only for
>>> the
>>> trampoline itself? Does the whole 1MB need to be marked X?
>>
>> The previous code was doing that.
> 
> So why not call set_memory_x() in your patch? Mind trying that?
> 

OK, there seems to be considerable duplication between
static_protections() and local invocation.  Consider PCI BIOS, which is
another X-needed region.

In static_protections() we have:

>         /*
>          * The BIOS area between 640k and 1Mb needs to be executable for
>          * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
>          */
> #ifdef CONFIG_PCI_BIOS
>         if (pcibios_enabled && within(pfn, BIOS_BEGIN >> PAGE_SHIFT, BIOS_END >> PAGE_SHIFT))
>                 pgprot_val(forbidden) |= _PAGE_NX;
> #endif

... however, in arch/x86/pci/pcbios.c:

> static inline void set_bios_x(void)
> {
>         pcibios_enabled = 1;
>         set_memory_x(PAGE_OFFSET + BIOS_BEGIN, (BIOS_END - BIOS_BEGIN) >> PAGE_SHIFT);
>         if (__supported_pte_mask & _PAGE_NX)
>                 printk(KERN_INFO "PCI : PCI BIOS aera is rw and x. Use pci=nobios if you want it NX.\n");
> }

This is blatant and insanely ugly code duplication!  In particular,
static_protections() is "action at a distance" which has no business
existing at all.

What I want to know is if static_protections() can somehow override
set_bios_x() in this context (in which case it's a serious design
error), or if it is plain redundant -- in the latter case we should
simply use the same technique elsewhere.

	-hpa

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/01/2011 10:26 PM, Ingo Molnar wrote:
> 
> So why not call set_memory_x() in your patch? Mind trying that?
> 
> Thanks,
> 
> 	Ingo

So I just tried that... it doesn't work.  The resulting pages still end
up NX:

---[ Kernel Mapping ]---
0xc0000000-0xc00a0000         640K     RW             GLB NX pte

This implies that the NX protection is applied after these allocations
happen, which is probably why the ugly hack in static_protections() to
set the PCI BIOS +x is there as well.

I have to admit to being rather at a loss for where in the boot sequence
the NX mappings get set up, despite staring at the code for some time.
mark_nxdata_nx() seems to only mark the kernel data/rodata and init
region... even though the latter is already done in free_init_pages().
It is also run way, way, way too late in the process -- why on earth
should we not have these protections during the driver initialization
phase of booting?

I talked to Suresh about the whole static_protections() bit, and as far
as he recalls it is because the entire set_memory_*() interface is
misdesigned to work on all aliases of a page, despite the fact that
protections are per mapping, not per physical page.

However, this isn't something we can fix for .38... I suspect
unprotecting the entire 0-640K region might just make most sense, and
then we need to do some serious restructuring of the entire handling of
this stuff, because it's broken seven ways to Sunday.

	-hpa

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[castet.matthieu]

[-- Attachment #1: Type: text/plain, Size: 1310 bytes --]

Selon "H. Peter Anvin" <hpa@zytor.com>:

> On 02/01/2011 10:26 PM, Ingo Molnar wrote:
> >
> > So why not call set_memory_x() in your patch? Mind trying that?
> >
> > Thanks,
> >
> > 	Ingo
>
> So I just tried that... it doesn't work.  The resulting pages still end
> up NX:
>
> ---[ Kernel Mapping ]---
> 0xc0000000-0xc00a0000         640K     RW             GLB NX pte
>
> This implies that the NX protection is applied after these allocations
> happen, which is probably why the ugly hack in static_protections() to
> set the PCI BIOS +x is there as well.
You could remove PCI BIOS +x hack in static protection, and the x mapping will
be set by set_memory_x().

The problem is that acpi_reserve_wakeup_memory is called too early, before we
build the page table with kernel_physical_mapping_init.

Doing the setting in a arch_initcall make it work.

>
> I talked to Suresh about the whole static_protections() bit, and as far
> as he recalls it is because the entire set_memory_*() interface is
> misdesigned to work on all aliases of a page, despite the fact that
> protections are per mapping, not per physical page.
The only stuff I understood of static_protections is the comment on top of it :
mapping of bios/kernel region should be on it otherwise, some callers can mess
the protection flags.


Matthieu

[-- Attachment #2: diff --]
[-- Type: application/octet-stream, Size: 705 bytes --]

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 69fd72a..1fde6ec 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -15,6 +15,7 @@
 
 #ifdef CONFIG_X86_32
 #include <asm/pgtable.h>
+#include <asm/cacheflush.h>
 #endif
 
 #include "realmode/wakeup.h"
@@ -149,6 +150,15 @@ void __init acpi_reserve_wakeup_memory(void)
 	memblock_x86_reserve_range(mem, mem + WAKEUP_SIZE, "ACPI WAKEUP");
 }
 
+#ifdef CONFIG_X86_32
+int __init acpi_configure_wakeup_memory(void)
+{
+	set_memory_x(acpi_realmode, (WAKEUP_SIZE) >> PAGE_SHIFT);
+	return 0;
+}
+arch_initcall(acpi_configure_wakeup_memory);
+#endif
+
 
 static int __init acpi_sleep_setup(char *str)
 {

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/05/2011 08:46 AM, castet.matthieu@free.fr wrote:
> You could remove PCI BIOS +x hack in static protection, and the x mapping will
> be set by set_memory_x().
> 
> The problem is that acpi_reserve_wakeup_memory is called too early, before we
> build the page table with kernel_physical_mapping_init.
> 
> Doing the setting in a arch_initcall make it work.

No, the problem is that the code is braindamaged and don't take into
account reserved areas or have a mechanism for marking the reserved
areas so that kernel_physical_mapping_init can do the right thing... and
then it's hacked around instead of done properly.

We obviously need to reserve this memory very early in order to make
sure it exists, and init_memory_mapping() ->
kernel_physical_mapping_init() really should be able to deal with that
(for example by walking the list of reserved memory regions and look
which ones of them should have specific protection bits -- not just NX
-- set appropriately.)

The trampoline unification patch could have made this less broken, but
that code is certainly not ready for .38.

I'm officially proposing that we mark the low 1 MiB +x for .38, and do
something saner for .39, including my finishing the trampoline
unification code.

Ingo: ok with you?

	-hpa

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

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/05/2011 08:46 AM, castet.matthieu@free.fr wrote:
> 
> Doing the setting in a arch_initcall make it work.
> 

Thinking about it, it's probably the sanest thing to do in the very
short term.  I don't get the #ifdef CONFIG_X86_32 bit in your patch,
though.  We reserve this area even on 64 bits, and so we should
logically do the same thing, if nothing else to avoid gratuitous
differences.

	-hpa

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

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

[-- Attachment #1: Type: text/plain, Size: 285 bytes --]

Here is my cleaned-up version of Matthieu's patch... please test so we
can commit it as soon as possible.

If this one doesn't work either it is time to totally revert.

	-hpa

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



[-- Attachment #2: 0001-x86-nx-Mark-the-ACPI-resume-trampoline-code-as-x.patch --]
[-- Type: text/x-patch, Size: 1572 bytes --]

>From 0b327f122ccfbbb28f2c22834916f12d03fc2a06 Mon Sep 17 00:00:00 2001
From: H. Peter Anvin <hpa@zytor.com>
Date: Sun, 6 Feb 2011 19:58:38 -0800
Subject: [PATCH] x86, nx: Mark the ACPI resume trampoline code as +x

We reserve lowmem for the things that need it, like the ACPI wakeup
code, way early to guarantee availability.  This happens before we set
up the proper pagetables, so set_memory_x() has no effect.

Until we have a better solution, use an initcall to mark the wakeup
code executable.

Originally-by: Matthieu Castet <castet.matthieu@free.fr>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
LKML-Reference: <1296924395.4d4d7eeb6f1fe@imp.free.fr>
---
 arch/x86/kernel/acpi/sleep.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 4d9ebba..68d1537 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -12,10 +12,8 @@
 #include <linux/cpumask.h>
 #include <asm/segment.h>
 #include <asm/desc.h>
-
-#ifdef CONFIG_X86_32
 #include <asm/pgtable.h>
-#endif
+#include <asm/cacheflush.h>
 
 #include "realmode/wakeup.h"
 #include "sleep.h"
@@ -149,6 +147,15 @@ void __init acpi_reserve_wakeup_memory(void)
 	memblock_x86_reserve_range(mem, mem + WAKEUP_SIZE, "ACPI WAKEUP");
 }
 
+int __init acpi_configure_wakeup_memory(void)
+{
+	if (acpi_realmode)
+		set_memory_x(acpi_realmode, WAKEUP_SIZE >> PAGE_SHIFT);
+
+	return 0;
+}
+arch_initcall(acpi_configure_wakeup_memory);
+
 
 static int __init acpi_sleep_setup(char *str)
 {
-- 
1.7.4

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> The trampoline unification patch could have made this less broken, but
> that code is certainly not ready for .38.
> 
> I'm officially proposing that we mark the low 1 MiB +x for .38, and do
> something saner for .39, including my finishing the trampoline
> unification code.
> 
> Ingo: ok with you?

Sure thing, sounds sane!

Thanks,

	Ingo

[tip:x86/urgent] x86, nx: Mark the ACPI resume trampoline code as +x

[tip-bot for H. Peter Anvin]

Commit-ID:  d344e38b2c151ca5e5e39f562017127e93912528
Gitweb:     http://git.kernel.org/tip/d344e38b2c151ca5e5e39f562017127e93912528
Author:     H. Peter Anvin <hpa@zytor.com>
AuthorDate: Sun, 6 Feb 2011 21:16:09 -0800
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Mon, 7 Feb 2011 09:07:13 +0100

x86, nx: Mark the ACPI resume trampoline code as +x

We reserve lowmem for the things that need it, like the ACPI
wakeup code, way early to guarantee availability.  This happens
before we set up the proper pagetables, so set_memory_x() has no
effect.

Until we have a better solution, use an initcall to mark the
wakeup code executable.

Originally-by: Matthieu Castet <castet.matthieu@free.fr>
Signed-off-by: H. Peter Anvin <hpa@zytor.com>
Cc: Matthias Hopf <mhopf@suse.de>
Cc: rjw@sisk.pl
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
LKML-Reference: <4D4F8019.2090104@zytor.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/kernel/acpi/sleep.c |   13 ++++++++++---
 1 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
index 4d9ebba..68d1537 100644
--- a/arch/x86/kernel/acpi/sleep.c
+++ b/arch/x86/kernel/acpi/sleep.c
@@ -12,10 +12,8 @@
 #include <linux/cpumask.h>
 #include <asm/segment.h>
 #include <asm/desc.h>
-
-#ifdef CONFIG_X86_32
 #include <asm/pgtable.h>
-#endif
+#include <asm/cacheflush.h>
 
 #include "realmode/wakeup.h"
 #include "sleep.h"
@@ -149,6 +147,15 @@ void __init acpi_reserve_wakeup_memory(void)
 	memblock_x86_reserve_range(mem, mem + WAKEUP_SIZE, "ACPI WAKEUP");
 }
 
+int __init acpi_configure_wakeup_memory(void)
+{
+	if (acpi_realmode)
+		set_memory_x(acpi_realmode, WAKEUP_SIZE >> PAGE_SHIFT);
+
+	return 0;
+}
+arch_initcall(acpi_configure_wakeup_memory);
+
 
 static int __init acpi_sleep_setup(char *str)
 {

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Matthias Hopf]

On Feb 06, 11 21:16:09 -0800, H. Peter Anvin wrote:
> Here is my cleaned-up version of Matthieu's patch... please test so we
> can commit it as soon as possible.

This works as well, tested against Linus' master from today (which
doesn't work).

Tested-By: Matthias Hopf <mhopf@suse.de>

> From 0b327f122ccfbbb28f2c22834916f12d03fc2a06 Mon Sep 17 00:00:00 2001
> From: H. Peter Anvin <hpa@zytor.com>
> Date: Sun, 6 Feb 2011 19:58:38 -0800
> Subject: [PATCH] x86, nx: Mark the ACPI resume trampoline code as +x

Matthias

-- 
Matthias Hopf <mhopf@suse.de>      __        __   __
Maxfeldstr. 5 / 90409 Nuernberg   (_   | |  (_   |__          mat@mshopf.de
Phone +49-911-74053-715           __)  |_|  __)  |__  R & D   www.mshopf.de

Re: [tip:x86/urgent] x86, nx: Mark the ACPI resume trampoline code as +x - fixed

[Marc Koschewski]

* tip-bot for H. Peter Anvin <hpa@zytor.com> [2011-02-07 09:24:55 +0000]:

Great! That one fixes ACPI S3 resume on i7.

Regards,
Marc

> Commit-ID:  d344e38b2c151ca5e5e39f562017127e93912528
> Gitweb:     http://git.kernel.org/tip/d344e38b2c151ca5e5e39f562017127e93912528
> Author:     H. Peter Anvin <hpa@zytor.com>
> AuthorDate: Sun, 6 Feb 2011 21:16:09 -0800
> Committer:  Ingo Molnar <mingo@elte.hu>
> CommitDate: Mon, 7 Feb 2011 09:07:13 +0100
> 
> x86, nx: Mark the ACPI resume trampoline code as +x
> 
> We reserve lowmem for the things that need it, like the ACPI
> wakeup code, way early to guarantee availability.  This happens
> before we set up the proper pagetables, so set_memory_x() has no
> effect.
> 
> Until we have a better solution, use an initcall to mark the
> wakeup code executable.
> 
> Originally-by: Matthieu Castet <castet.matthieu@free.fr>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
> Cc: Matthias Hopf <mhopf@suse.de>
> Cc: rjw@sisk.pl
> Cc: Suresh Siddha <suresh.b.siddha@intel.com>
> LKML-Reference: <4D4F8019.2090104@zytor.com>
> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> ---
>  arch/x86/kernel/acpi/sleep.c |   13 ++++++++++---
>  1 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/acpi/sleep.c b/arch/x86/kernel/acpi/sleep.c
> index 4d9ebba..68d1537 100644
> --- a/arch/x86/kernel/acpi/sleep.c
> +++ b/arch/x86/kernel/acpi/sleep.c
> @@ -12,10 +12,8 @@
>  #include <linux/cpumask.h>
>  #include <asm/segment.h>
>  #include <asm/desc.h>
> -
> -#ifdef CONFIG_X86_32
>  #include <asm/pgtable.h>
> -#endif
> +#include <asm/cacheflush.h>
>  
>  #include "realmode/wakeup.h"
>  #include "sleep.h"
> @@ -149,6 +147,15 @@ void __init acpi_reserve_wakeup_memory(void)
>  	memblock_x86_reserve_range(mem, mem + WAKEUP_SIZE, "ACPI WAKEUP");
>  }
>  
> +int __init acpi_configure_wakeup_memory(void)
> +{
> +	if (acpi_realmode)
> +		set_memory_x(acpi_realmode, WAKEUP_SIZE >> PAGE_SHIFT);
> +
> +	return 0;
> +}
> +arch_initcall(acpi_configure_wakeup_memory);
> +
>  
>  static int __init acpi_sleep_setup(char *str)
>  {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 

-- 
Marc Koschewski

Re: [tip:x86/urgent] x86, nx: Mark the ACPI resume trampoline code as +x - fixed

[Ingo Molnar]

* Marc Koschewski <marc@osknowledge.org> wrote:

> * tip-bot for H. Peter Anvin <hpa@zytor.com> [2011-02-07 09:24:55 +0000]:
> 
> Great! That one fixes ACPI S3 resume on i7.

Ok, great - i'm sending it to Linus then.

Thanks,

	Ingo

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[castet.matthieu]

Thanks for cleaning the patch.

Selon "H. Peter Anvin" <hpa@zytor.com>:
> On 02/05/2011 08:46 AM, castet.matthieu@free.fr wrote:
>
> No, the problem is that the code is braindamaged and don't take into
> account reserved areas or have a mechanism for marking the reserved
> areas so that kernel_physical_mapping_init can do the right thing... and
> then it's hacked around instead of done properly.
>
> We obviously need to reserve this memory very early in order to make
> sure it exists, and init_memory_mapping() ->
> kernel_physical_mapping_init() really should be able to deal with that
> (for example by walking the list of reserved memory regions and look
> which ones of them should have specific protection bits -- not just NX
> -- set appropriately.)
>
> The trampoline unification patch could have made this less broken, but
> that code is certainly not ready for .38.
>
For .39 I hope we could remove most of the RWX rights after init (This means
make low memory trampoline NX or !RW).
This should be possible on :
- 32 bit if wakeup use trampoline_32 [1] that doesn't enable paging in low
memory (can be NX)
- trampoline_64 need fix to support NX on data section. It tries to read data
section before enabling NX. A possible fix is to use its own page table [2]. And
the kernel one can be NX.


Matthieu


[1]
http://marc.info/?l=linux-acpi&m=129616540303575&w=2

[2]
http://marc.info/?l=linux-kernel&m=129590778414274&w=2

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/07/2011 11:59 AM, castet.matthieu@free.fr wrote:
> For .39 I hope we could remove most of the RWX rights after init (This means
> make low memory trampoline NX or !RW).
> This should be possible on :
> - 32 bit if wakeup use trampoline_32 [1] that doesn't enable paging in low
> memory (can be NX)
> - trampoline_64 need fix to support NX on data section. It tries to read data
> section before enabling NX. A possible fix is to use its own page table [2]. And
> the kernel one can be NX.

No, you're really barking down the wrong path on this.  The trampoline
code is tiny; I don't think it is really worth trying to NX-ify it.  The
additional complexity caused by not being able to execute in this space
will really damage some other incoming code, so it isn't an option as
far as I'm concerned.

	-hpa

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/07/2011 11:59 AM, castet.matthieu@free.fr wrote:
> For .39 I hope we could remove most of the RWX rights after init (This means
> make low memory trampoline NX or !RW).
> This should be possible on :
> - 32 bit if wakeup use trampoline_32 [1] that doesn't enable paging in low
> memory (can be NX)
> - trampoline_64 need fix to support NX on data section. It tries to read data
> section before enabling NX. A possible fix is to use its own page table [2]. And
> the kernel one can be NX.

What *should* happen -- ideally for .39 -- is that NX (and RO!)
protection should be done per linear mapping, not per physical page.  A
page that is mapped more than once is mapped for a different purpose,
and as such probably should have different permissions.  A lot of the
static_protections() garbage is about enforcing those as exceptions, but
let's face it, that should be the *norm*.

	-hpa

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[matthieu castet]

H. Peter Anvin a écrit :
> On 02/07/2011 11:59 AM, castet.matthieu@free.fr wrote:
>> For .39 I hope we could remove most of the RWX rights after init (This means
>> make low memory trampoline NX or !RW).
>> This should be possible on :
>> - 32 bit if wakeup use trampoline_32 [1] that doesn't enable paging in low
>> memory (can be NX)
>> - trampoline_64 need fix to support NX on data section. It tries to read data
>> section before enabling NX. A possible fix is to use its own page table [2]. And
>> the kernel one can be NX.
> 
> No, you're really barking down the wrong path on this.  The trampoline
> code is tiny; I don't think it is really worth trying to NX-ify it.  The
Even if the trampoline is tiny, a hole is a hole.

The trampoline code job is to jump from low memory (realmode) to somewhere in kernel text.
Why should we enable paging or use kernel page table for doing that ?

> additional complexity caused by not being able to execute in this space
> will really damage some other incoming code, so it isn't an option as
> far as I'm concerned.
> 
What do you plan to add that won't be compatible with that ?


Matthieu

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/12/2011 08:10 AM, matthieu castet wrote:
>>
>> No, you're really barking down the wrong path on this.  The trampoline
>> code is tiny; I don't think it is really worth trying to NX-ify it.  The
> Even if the trampoline is tiny, a hole is a hole.
> 
> The trampoline code job is to jump from low memory (realmode) to
> somewhere in kernel text.
> Why should we enable paging or use kernel page table for doing that ?
> 

That's not the problem.  The problem is that most of the "trampoline
codes" need parameters which need to be written by the kernel before
invocation.  Separating the address spaces out into text and
writable-for-the-kernel data is possible, but messy since the individual
chunks each have different addressing constraints.  I tried to get them
into the same link, but that has turned out to be very difficult and
probably will require a fair bit of restructuring, especially the code
shared with the boot code.

	-hpa


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

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/07/2011 11:59 AM, castet.matthieu@free.fr wrote:
>
> For .39 I hope we could remove most of the RWX rights after init (This means
> make low memory trampoline NX or !RW).

By the way, I think this is the wrong goal.  I think we should have
things enabled at their lowest permission level *as early as possible*.
 The current model of tightening down permissions late in the boot is
really the wrong model.

	-hpa

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

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Rafael J. Wysocki]

On Monday, February 14, 2011, H. Peter Anvin wrote:
> On 02/07/2011 11:59 AM, castet.matthieu@free.fr wrote:
> >
> > For .39 I hope we could remove most of the RWX rights after init (This means
> > make low memory trampoline NX or !RW).
> 
> By the way, I think this is the wrong goal.  I think we should have
> things enabled at their lowest permission level *as early as possible*.
>  The current model of tightening down permissions late in the boot is
> really the wrong model.

FWIW, I agree.

Rafael

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Pavel Machek]

On Sat 2011-02-12 17:10:14, matthieu castet wrote:
> H. Peter Anvin a écrit :
> >On 02/07/2011 11:59 AM, castet.matthieu@free.fr wrote:
> >>For .39 I hope we could remove most of the RWX rights after init (This means
> >>make low memory trampoline NX or !RW).
> >>This should be possible on :
> >>- 32 bit if wakeup use trampoline_32 [1] that doesn't enable paging in low
> >>memory (can be NX)
> >>- trampoline_64 need fix to support NX on data section. It tries to read data
> >>section before enabling NX. A possible fix is to use its own page table [2]. And
> >>the kernel one can be NX.
> >
> >No, you're really barking down the wrong path on this.  The trampoline
> >code is tiny; I don't think it is really worth trying to NX-ify it.  The
> Even if the trampoline is tiny, a hole is a hole.

What kind of hole are you talking about?

If evil code is running in ring0, you already lost.

I doubt you can remove all the trampolines, and I do not think you
should uglify the kernel trying to do that.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[H. Peter Anvin]

On 02/06/2011 02:30 AM, Bodo Eggert wrote:
> H. Peter Anvin <hpa@zytor.com> wrote:
> 
>> In static_protections() we have:
>>
>>>         /*
>>>          * The BIOS area between 640k and 1Mb needs to be executable for
>>>          * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
>>>          */
> 
> I don't think the CGA/MDA/VGA graphics memory areas should be executable,
> and I doubt execute access to these areas is required - is it?
> 
> 0xA000:0000 might be a BIOS area, if it is, you don't have a VGA and the
> ROM will be exactly 64K. 0xB800 and 0xB000 SHOULD NOT be a ROM, nobody dared
> to use it (I don't remember exactly where ROMs are searched).


0xA0000..0xBFFFF can be ROM if you don't have a VGA card; this is new in
PCI 3.x IIRC.  However, in legacy systems you will not have ROM in this
area.

	-hpa

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

Re: [PATCH] NX protection for kernel data : fix 32 bits S3 suspend

[Bodo Eggert]

H. Peter Anvin <hpa@zytor.com> wrote:

> In static_protections() we have:
> 
>>         /*
>>          * The BIOS area between 640k and 1Mb needs to be executable for
>>          * PCI BIOS based config access (CONFIG_PCI_GOBIOS) support.
>>          */

I don't think the CGA/MDA/VGA graphics memory areas should be executable,
and I doubt execute access to these areas is required - is it?

0xA000:0000 might be a BIOS area, if it is, you don't have a VGA and the
ROM will be exactly 64K. 0xB800 and 0xB000 SHOULD NOT be a ROM, nobody dared
to use it (I don't remember exactly where ROMs are searched).
-- 
Funny quotes:
34. If FedEx and UPS were to merge, would they call it Fed UP?

Friß, Spammer: igc@vhFgi.7eggert.dyndns.org fdnJu@jQOez.7eggert.dyndns.org