* matthieu castet <[email protected]> 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
Quoting Ingo Molnar <[email protected]>:
>
> * matthieu castet <[email protected]> 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
On 02/01/2011 05:25 AM, [email protected] 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
* [email protected] <[email protected]> wrote:
> Quoting Ingo Molnar <[email protected]>:
>
> >
> > * matthieu castet <[email protected]> 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
On 02/01/2011 10:26 PM, Ingo Molnar wrote:
>
> * [email protected] <[email protected]> wrote:
>
>> Quoting Ingo Molnar <[email protected]>:
>>
>>>
>>> * matthieu castet <[email protected]> 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
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
Selon "H. Peter Anvin" <[email protected]>:
> 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
On 02/05/2011 08:46 AM, [email protected] 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.
On 02/05/2011 08:46 AM, [email protected] 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.
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.
* H. Peter Anvin <[email protected]> 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
Commit-ID: d344e38b2c151ca5e5e39f562017127e93912528
Gitweb: http://git.kernel.org/tip/d344e38b2c151ca5e5e39f562017127e93912528
Author: H. Peter Anvin <[email protected]>
AuthorDate: Sun, 6 Feb 2011 21:16:09 -0800
Committer: Ingo Molnar <[email protected]>
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 <[email protected]>
Signed-off-by: H. Peter Anvin <[email protected]>
Cc: Matthias Hopf <[email protected]>
Cc: [email protected]
Cc: Suresh Siddha <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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)
{
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 <[email protected]>
> From 0b327f122ccfbbb28f2c22834916f12d03fc2a06 Mon Sep 17 00:00:00 2001
> From: H. Peter Anvin <[email protected]>
> Date: Sun, 6 Feb 2011 19:58:38 -0800
> Subject: [PATCH] x86, nx: Mark the ACPI resume trampoline code as +x
Matthias
--
Matthias Hopf <[email protected]> __ __ __
Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ [email protected]
Phone +49-911-74053-715 __) |_| __) |__ R & D http://www.mshopf.de
* tip-bot for H. Peter Anvin <[email protected]> [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 <[email protected]>
> AuthorDate: Sun, 6 Feb 2011 21:16:09 -0800
> Committer: Ingo Molnar <[email protected]>
> 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 <[email protected]>
> Signed-off-by: H. Peter Anvin <[email protected]>
> Cc: Matthias Hopf <[email protected]>
> Cc: [email protected]
> Cc: Suresh Siddha <[email protected]>
> LKML-Reference: <[email protected]>
> Signed-off-by: Ingo Molnar <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
--
Marc Koschewski
* Marc Koschewski <[email protected]> wrote:
> * tip-bot for H. Peter Anvin <[email protected]> [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
Thanks for cleaning the patch.
Selon "H. Peter Anvin" <[email protected]>:
> On 02/05/2011 08:46 AM, [email protected] 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
On 02/07/2011 11:59 AM, [email protected] 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
On 02/07/2011 11:59 AM, [email protected] 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
H. Peter Anvin a ?crit :
> On 02/07/2011 11:59 AM, [email protected] 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
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.
On 02/07/2011 11:59 AM, [email protected] 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.
On Monday, February 14, 2011, H. Peter Anvin wrote:
> On 02/07/2011 11:59 AM, [email protected] 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
On Sat 2011-02-12 17:10:14, matthieu castet wrote:
> H. Peter Anvin a ?crit :
> >On 02/07/2011 11:59 AM, [email protected] 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