-tip tree resume fail, bisect to 5bd5a45(x86: Add NX protection for kernel data)

DISCUSSION Lin Ming 7 participants 24 messages Nov 22, 2010 – Mar 10, 2011

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Lin Ming

author

2010-11-22 07:22 UTC #0 lore raw

Hi, all

Current -tip tree(92c883a) fail to resume after suspend to mem. Bisect to commit 5bd5a45(x86: Add NX protection for kernel data).

commit 5bd5a452662bc37c54fb6828db1a3faf87e6511c

Author: Matthieu Castet <[email protected]>
Date:   Tue Nov 16 22:31:26 2010 +0100

x86: Add NX protection for kernel data

I did some debug and found the regression is caused by below line.

- set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+ set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);

In my machine,
rodata_start=0xffffffff81a00000, end=0xffffffff81e00000, kernel_end=0xffffffff82200000

Without this commit, kernel page table dumps

---[ High Kernel Mapping ]---
....

0xffffffff81a00000-0xffffffff81c00000           2M     ro         PSE GLB NX pmd
0xffffffff81c00000-0xffffffff81d91000        1604K     ro             GLB NX pte
0xffffffff81d91000-0xffffffff81e00000         444K     ro             GLB NX pte
0xffffffff81e00000-0xffffffff82000000           2M     RW         PSE GLB x  pmd
0xffffffff82000000-0xffffffff8200c000          48K     RW             GLB x  pte
0xffffffff8200c000-0xffffffff82100000         976K     RW             GLB x  pte
0xffffffff82100000-0xffffffff82200000           1M     RW             GLB x  pte

.....

With this commit, kernel page table dumps

---[ High Kernel Mapping ]---
....

0xffffffff81a00000-0xffffffff81c00000           2M     ro         PSE GLB NX pmd
0xffffffff81c00000-0xffffffff81d91000        1604K     ro             GLB NX pte
0xffffffff81d91000-0xffffffff81e00000         444K     ro             GLB NX pte
0xffffffff81e00000-0xffffffff82000000           2M     RW         PSE GLB NX pmd
0xffffffff82000000-0xffffffff8200c000          48K     RW             GLB NX pte
0xffffffff8200c000-0xffffffff82100000         976K     RW             GLB NX pte
0xffffffff82100000-0xffffffff82200000           1M     RW             GLB NX pte

.....

The only difference is 0xffffffff81e00000(end) to 0xffffffff82200000(kernel_end) is set to NX.

I guess this range code is used by resume wakeup code. So setting it to NX causes problem.

Any idea?

Thanks,
Lin Ming

Ingo Molnar

2010-11-22 08:07 UTC #1 lore raw

Quoted 44 lines · Lin Ming

Hi, all

Current -tip tree(92c883a) fail to resume after suspend to mem. Bisect to commit 5bd5a45(x86: Add NX protection for kernel data).

commit 5bd5a452662bc37c54fb6828db1a3faf87e6511c

Author: Matthieu Castet <[email protected]>
Date:   Tue Nov 16 22:31:26 2010 +0100

x86: Add NX protection for kernel data

I did some debug and found the regression is caused by below line.

(quoted diff) +1 -1

-       set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+       set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);

In my machine,
rodata_start=0xffffffff81a00000, end=0xffffffff81e00000, kernel_end=0xffffffff82200000

Without this commit, kernel page table dumps

---[ High Kernel Mapping ]---
....

0xffffffff81a00000-0xffffffff81c00000           2M     ro         PSE GLB NX pmd
0xffffffff81c00000-0xffffffff81d91000        1604K     ro             GLB NX pte
0xffffffff81d91000-0xffffffff81e00000         444K     ro             GLB NX pte
0xffffffff81e00000-0xffffffff82000000           2M     RW         PSE GLB x  pmd
0xffffffff82000000-0xffffffff8200c000          48K     RW             GLB x  pte
0xffffffff8200c000-0xffffffff82100000         976K     RW             GLB x  pte
0xffffffff82100000-0xffffffff82200000           1M     RW             GLB x  pte

.....

With this commit, kernel page table dumps

---[ High Kernel Mapping ]---
....

0xffffffff81a00000-0xffffffff81c00000           2M     ro         PSE GLB NX pmd
0xffffffff81c00000-0xffffffff81d91000        1604K     ro             GLB NX pte
0xffffffff81d91000-0xffffffff81e00000         444K     ro             GLB NX pte
0xffffffff81e00000-0xffffffff82000000           2M     RW         PSE GLB NX pmd
0xffffffff82000000-0xffffffff8200c000          48K     RW             GLB NX pte
0xffffffff8200c000-0xffffffff82100000         976K     RW             GLB NX pte
0xffffffff82100000-0xffffffff82200000           1M     RW             GLB NX pte

Quoted 7 lines

.....
The only difference is 0xffffffff81e00000(end) to 0xffffffff82200000(kernel_end) is set to NX.
I guess this range code is used by resume wakeup code. So setting it to NX causes problem.
Any idea?

Yeah, the resume area is a trampoline, it needs to be executable.

Thanks,

Ingo

Andi Kleen

2010-11-22 09:20 UTC #2 lore raw

Quoted 11 lines · Ingo Molnar

Quoted 11 lines

---[ High Kernel Mapping ]---
....

0xffffffff81a00000-0xffffffff81c00000           2M     ro         PSE GLB NX pmd
0xffffffff81c00000-0xffffffff81d91000        1604K     ro             GLB NX pte
0xffffffff81d91000-0xffffffff81e00000         444K     ro             GLB NX pte
0xffffffff81e00000-0xffffffff82000000           2M     RW         PSE GLB NX pmd
0xffffffff82000000-0xffffffff8200c000          48K     RW             GLB NX pte
0xffffffff8200c000-0xffffffff82100000         976K     RW             GLB NX pte
0xffffffff82100000-0xffffffff82200000           1M     RW             GLB NX pte

.....

Quoted 7 lines

Quoted 5 lines
The only difference is 0xffffffff81e00000(end) to 0xffffffff82200000(kernel_end) is set to NX.
I guess this range code is used by resume wakeup code. So setting it to NX causes problem.
Any idea?
Yeah, the resume area is a trampoline, it needs to be executable.

Also it looks like most of the kernel linker supplied data is now not 2MB mapped anymore? That will surely cost TLB entries.

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

[email protected]

2010-11-22 10:29 UTC #3 lore raw

Hi Andi,

Quoting Andi Kleen <[email protected]>:

Quoted 23 lines · Ingo Molnar

Quoted 3 lines
Quoted 3 lines
---[ High Kernel Mapping ]---
....
0xffffffff81a00000-0xffffffff81c00000 2M ro PSE GLB
NX pmd
Quoted 1 line
Quoted 1 line
0xffffffff81c00000-0xffffffff81d91000 1604K ro GLB
NX pte
Quoted 1 line
Quoted 1 line
0xffffffff81d91000-0xffffffff81e00000 444K ro GLB
NX pte
Quoted 1 line
Quoted 1 line
0xffffffff81e00000-0xffffffff82000000 2M RW PSE GLB
NX pmd
Quoted 1 line
Quoted 1 line
0xffffffff82000000-0xffffffff8200c000 48K RW GLB
NX pte
Quoted 1 line
Quoted 1 line
0xffffffff8200c000-0xffffffff82100000 976K RW GLB
NX pte
Quoted 1 line
Quoted 1 line
0xffffffff82100000-0xffffffff82200000 1M RW GLB
NX pte
Quoted 3 lines
Quoted 3 lines
.....
The only difference is 0xffffffff81e00000(end) to
0xffffffff82200000(kernel_end) is set to NX.
Quoted 2 lines
Quoted 2 lines
I guess this range code is used by resume wakeup code. So setting it to

Quoted 9 lines

NX causes problem.
Quoted 4 lines
Quoted 2 lines
Any idea?
Yeah, the resume area is a trampoline, it needs to be executable.
Also it looks like most of the kernel linker supplied data is now not 2MB mapped anymore? That will surely cost TLB entries.

Any idea where it comes from ?

The mapping is still not 2MB mapped without the commit.

The patch only add

+#if defined(CONFIG_DEBUG_RODATA)
+ /* .text should occupy whole number of pages */
+ . = ALIGN(PAGE_SIZE);
+#endif
X64_ALIGN_DEBUG_RODATA_BEGIN
RO_DATA(PAGE_SIZE)
X64_ALIGN_DEBUG_RODATA_END

But X64_ALIGN_DEBUG_RODATA_BEGIN is aligned on 2M, so it shouldn't change nothing on x64 ?

Matthieu

Andi Kleen

2010-11-22 10:47 UTC #4 lore raw

Quoted 3 lines

But X64_ALIGN_DEBUG_RODATA_BEGIN is aligned on 2M, so it shouldn't change nothing on x64 ?

Yes sorry for the confusion. That's unrelated to your patch and already happened earlier. I just noticed it while looking at the page table dumps in the report.

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

Peter Zijlstra

maintainer

2010-11-22 13:03 UTC #5 lore raw

In reply to Lin Ming

Quoted 4 lines · Lin Ming

Hi, all
Current -tip tree(92c883a) fail to resume after suspend to mem. Bisect to commit 5bd5a45(x86: Add NX protection for kernel data).

Drad, if only I'd looked at LKML before I did the bisection myself..

Review

Quoted 11 lines

commit 5bd5a452662bc37c54fb6828db1a3faf87e6511c
Author: Matthieu Castet <[email protected]>
Date:   Tue Nov 16 22:31:26 2010 +0100
x86: Add NX protection for kernel data
I did some debug and found the regression is caused by below line.
(quoted diff) +1 -1
-       set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+       set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);

Confirmed, with that hunk reverted my machine works again..

---
 arch/x86/mm/init_64.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

arch/x86/mm/init_64.c +1 -2

index ce59c05..71a5929 100644
@@ -788,7 +788,6 @@ void mark_rodata_ro(void)
 	unsigned long rodata_start =
 		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
-	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
 	unsigned long data_start = (unsigned long) &_sdata;

@@ -803,7 +802,7 @@ void mark_rodata_ro(void)
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();

[email protected]

2010-11-22 16:29 UTC #6 lore raw

Hi,

Quoting Peter Zijlstra <[email protected]>:

Quoted 15 lines · Lin Ming

Quoted 4 lines
Hi, all
Current -tip tree(92c883a) fail to resume after suspend to mem. Bisect to commit 5bd5a45(x86: Add NX protection for kernel data).
Drad, if only I'd looked at LKML before I did the bisection myself..
Quoted 7 lines
commit 5bd5a452662bc37c54fb6828db1a3faf87e6511c
Author: Matthieu Castet <[email protected]>
Date:   Tue Nov 16 22:31:26 2010 +0100
x86: Add NX protection for kernel data

Quoted 9 lines

Review
Quoted 4 lines
I did some debug and found the regression is caused by below line.
(quoted diff) +1 -1
-       set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+       set_memory_nx(rodata_start, (kernel_end - rodata_start) >>
PAGE_SHIFT);
Confirmed, with that hunk reverted my machine works again..

Ok,
I will submit a proper fix :
in acpi_save_state_mem/acpi_restore_state_mem make the trampoline X or NX.

Matthieu

Peter Zijlstra

maintainer

2010-11-22 16:36 UTC #7 lore raw

Quoted 23 lines · On Mon, 2010-11-22 at 17:29 +0100, [email protected] wrote:

Hi,
Quoting Peter Zijlstra <[email protected]>:
Quoted 19 lines · Lin Ming
Quoted 4 lines
Hi, all
Current -tip tree(92c883a) fail to resume after suspend to mem. Bisect to commit 5bd5a45(x86: Add NX protection for kernel data).
Drad, if only I'd looked at LKML before I did the bisection myself..
Quoted 11 lines
commit 5bd5a452662bc37c54fb6828db1a3faf87e6511c
Author: Matthieu Castet <[email protected]>
Date:   Tue Nov 16 22:31:26 2010 +0100
x86: Add NX protection for kernel data
I did some debug and found the regression is caused by below line.
(quoted diff) +1 -1
-       set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+       set_memory_nx(rodata_start, (kernel_end - rodata_start) >>

Quoted 8 lines

Quoted 5 lines
PAGE_SHIFT);
Confirmed, with that hunk reverted my machine works again..
Ok,
I will submit a proper fix :
in acpi_save_state_mem/acpi_restore_state_mem make the trampoline X or NX.

That seems to be a S3 specific code path, that won't fix anything. Simply do:

echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;

and your machine will explode..

Andi Kleen

2010-11-22 16:42 UTC #8 lore raw

Quoted 7 lines

That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
and your machine will explode..

The SMP startup trampoline is copied I believe and only executed in real mode without page tables.

So it's perhaps not the trampoline, but the early startup code that ends up being broken.

-Andi

Signature · 1 line

[email protected] -- Speaking for myself only.

mat

2010-11-23 22:55 UTC #9 lore raw

Quoted 6 lines · Andi Kleen

Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;

Quoted 7 lines

Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.

yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).

So they can't end up int the kernel data mapping ?

So it should something else.

I will try to investigate on this.

Matthieu

mat

2010-11-26 17:31 UTC #10 lore raw

Quoted 20 lines · mat

Quoted 6 lines · Andi Kleen
Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
Quoted 7 lines
Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.
yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).

Quoted 6 lines

So they can't end up int the kernel data mapping ?
So it should something else.
I will try to investigate on this.

Unfortunately on my laptop supporting NX, suspend to ram seems broken (even without this patch) and I got only one core, so I am unable to test it.

Does cpu suspend/resume is broken ? Or it is only S3 ?

If yes, are there any interesting trace if we suspend only one core with sysfs.

Thanks,

Matthieu

Lin Ming

2010-11-26 23:39 UTC #11 lore raw

Quoted 29 lines · mat

Quoted 20 lines · mat
Quoted 6 lines · Andi Kleen
Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
Quoted 7 lines
Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.
yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).
Quoted 6 lines
So they can't end up int the kernel data mapping ?
So it should something else.
I will try to investigate on this.

Quoted 8 lines

Unfortunately on my laptop supporting NX, suspend to ram seems broken (even without this patch) and I got only one core, so I am unable to test it.
Does cpu suspend/resume is broken ? Or it is only S3 ?
If yes, are there any interesting trace if we suspend only one core with sysfs.

Hi,

I am on travel now, will test it when I'm back next week.

Thanks,
Lin Ming

Quoted 4 lines

Thanks,
Matthieu

Lin Ming

author

2010-11-30 04:58 UTC #12 lore raw

In reply to mat

Quoted 29 lines · mat

Quoted 20 lines · mat
Quoted 6 lines · Andi Kleen
Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
Quoted 7 lines
Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.
yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).
Quoted 6 lines
So they can't end up int the kernel data mapping ?
So it should something else.
I will try to investigate on this.

Quoted 8 lines

Unfortunately on my laptop supporting NX, suspend to ram seems broken (even without this patch) and I got only one core, so I am unable to test it.
Does cpu suspend/resume is broken ? Or it is only S3 ?
If yes, are there any interesting trace if we suspend only one core with sysfs.

echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;

then machine just reboots...

Quoted 4 lines

Thanks,
Matthieu

Peter Zijlstra

maintainer Tested-by

2010-11-30 11:27 UTC #13 lore raw

Quoted 5 lines · Lin Ming

echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...

That should work on current -tip (and my test machine confirms with a -tip of yesterday) due to:

---
commit 691513f70d3957939a318da970987b876c720861

Author: Lin Ming <[email protected]>
Date:   Mon Nov 22 14:03:28 2010 +0100

x86: Resume trampoline must be executable

commit 5bd5a452(x86: Add NX protection for kernel data) marked the trampoline area NX - which unsurprisingly breaks resume and cpu hotplug.

Revert the portion of that commit, which touches the trampoline.

Originally-from: Lin Ming <[email protected]>
LKML-Reference: <[email protected]>

Cc Matthieu Castet <[email protected]>

Cc Siarhei Liakh <[email protected]>

Cc Xuxian Jiang <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Arjan van de Ven <[email protected]>

Cc Andi Kleen <[email protected]>

Tested-by Peter Zijlstra <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

arch/x86/mm/init_64.c +1 -2

index ce59c05..71a5929 100644
@@ -788,7 +788,6 @@ void mark_rodata_ro(void)
 	unsigned long rodata_start =
 		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
-	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
 	unsigned long data_start = (unsigned long) &_sdata;

@@ -803,7 +802,7 @@ void mark_rodata_ro(void)
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();

Lin Ming

author

2010-12-01 00:12 UTC #14 lore raw

Quoted 9 lines · Peter Zijlstra

Quoted 5 lines · Lin Ming
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...
That should work on current -tip (and my test machine confirms with a -tip of yesterday) due to:

Yes, I know.

I tested for Mat's request to confirm that CPU suspend/resume also broken, not only S3.

And no interesting trace I can get because machine just reboots immediately when CPU suspend/resume.

Thanks.

Quoted 47 lines

---
commit 691513f70d3957939a318da970987b876c720861
Author: Lin Ming <[email protected]>
Date:   Mon Nov 22 14:03:28 2010 +0100
x86: Resume trampoline must be executable
commit 5bd5a452(x86: Add NX protection for kernel data) marked the trampoline area NX - which unsurprisingly breaks resume and cpu hotplug.
Revert the portion of that commit, which touches the trampoline.
Originally-from: Lin Ming <[email protected]>
LKML-Reference: <[email protected]>
Cc Matthieu Castet <[email protected]>
Cc Siarhei Liakh <[email protected]>
Cc Xuxian Jiang <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Arjan van de Ven <[email protected]>
Cc Andi Kleen <[email protected]>
Tested-by Peter Zijlstra <[email protected]>
Signed-off-by Thomas Gleixner <[email protected]>
arch/x86/mm/init_64.c +1 -2
index ce59c05..71a5929 100644
@@ -788,7 +788,6 @@ void mark_rodata_ro(void)
 	unsigned long rodata_start =
 		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
-	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
 	unsigned long data_start = (unsigned long) &_sdata;
@@ -803,7 +802,7 @@ void mark_rodata_ro(void)
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();
 

matthieu castet

2011-01-23 19:06 UTC #15 lore raw

Quoted 7 lines · Lin Ming

Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Lin Ming
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...

I tried to do the same thing on qemu, and the same behavior happened (ie reboot when resuming cpu1).

After enabling qemu log, I found that a triple fault was happening at the beginning of secondary_startup_64 when doing "addq phys_base(%rip), %rax".

Why ?
I suppose because we access data set to NX, but we don't have enabled yet NX in the msr. So the cpu crash due to "reserved bit check".

If we enable NX before reading data, there is no more crash (patch attached).

Now I am not sure this is the correct fix. I think the problem is that trampoline using kernel page table is very dangerous. The kernel can have modified them atfer booting !
May be all the paging stuff should have been done in head_64.S. A first one with identity mapping, and the second one for the real kernel stuff.

Matthieu

arch/x86/kernel/head_64.S +11 ~~-10~~

index 239046b..9f32295 100644
@@ -165,16 +165,6 @@ ENTRY(secondary_startup_64)
 	movl	$(X86_CR4_PAE | X86_CR4_PGE), %eax
 	movq	%rax, %cr4
 
-	/* Setup early boot stage 4 level pagetables. */
-	movq	$(init_level4_pgt - __START_KERNEL_map), %rax
-	addq	phys_base(%rip), %rax
-	movq	%rax, %cr3
-
-	/* Ensure I am executing from virtual addresses */
-	movq	$1f, %rax
-	jmp	*%rax
-1:
-
 	/* Check if nx is implemented */
 	movl	$0x80000001, %eax
 	cpuid

@@ -189,6 +179,17 @@ ENTRY(secondary_startup_64)
 	btsl	$_EFER_NX, %eax
 1:	wrmsr				/* Make changes effective */
 
+	/* Setup early boot stage 4 level pagetables. */
+	movq	$(init_level4_pgt - __START_KERNEL_map), %rax
+	addq	phys_base(%rip), %rax
+	movq	%rax, %cr3
+
+	/* Ensure I am executing from virtual addresses */
+	movq	$1f, %rax
+	jmp	*%rax
+1:
+
+
 	/* Setup cr0 */
 #define CR0_STATE	(X86_CR0_PE | X86_CR0_MP | X86_CR0_ET | \
 			 X86_CR0_NE | X86_CR0_WP | X86_CR0_AM | \

matthieu castet

2011-01-24 22:22 UTC #16 lore raw

Quoted 16 lines · matthieu castet

Quoted 7 lines · Lin Ming
Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Lin Ming
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...
I tried to do the same thing on qemu, and the same behavior happened (ie reboot when resuming cpu1).
After enabling qemu log, I found that a triple fault was happening at the beginning of secondary_startup_64 when doing "addq phys_base(%rip), %rax".
Why ?
I suppose because we access data set to NX, but we don't have enabled yet NX in the msr. So the cpu crash due to "reserved bit check".

Quoted 7 lines

If we enable NX before reading data, there is no more crash (patch attached).
Now I am not sure this is the correct fix. I think the problem is that trampoline using kernel page table is very dangerous. The kernel can have modified them atfer booting !
May be all the paging stuff should have been done in head_64.S. A first one with identity mapping, and the second one for the real kernel stuff.

Lin, could you try this patch on your x64 machine.

Thanks

Matthieu

Quoted 1 line

From c6d0a62d47be9956abb9d3e1b7c952eaa7e0df7e Mon Sep 17 00:00:00 2001

From: Matthieu CASTET <[email protected]>
Date: Mon, 24 Jan 2011 23:12:45 +0100
Subject: [PATCH] x86 : Add NX protection for kernel data on 64 bit

This fix the cpu hotplug support, by allocating dedicated page table for ident mapping in trampoline.
This is need because kernel set NX flag in level3_ident_pgt and level3_kernel_pgt, and made it unusable from trampoline.

We also set the Low kernel Mapping to NX.

Finaly we apply nx in free_init_pages only when we switch to NX mode in order to preserve large page mapping.

mapping now look like :
---[ Low Kernel Mapping ]---

0xffff880000000000-0xffff880000200000           2M     RW             GLB NX pte
0xffff880000200000-0xffff880001000000          14M     RW         PSE GLB NX pmd
0xffff880001000000-0xffff880001200000           2M     ro         PSE GLB NX pmd
0xffff880001200000-0xffff8800012ae000         696K     ro             GLB NX pte
0xffff8800012ae000-0xffff880001400000        1352K     RW             GLB NX pte
0xffff880001400000-0xffff880001503000        1036K     ro             GLB NX pte
0xffff880001503000-0xffff880001600000        1012K     RW             GLB NX pte
0xffff880001600000-0xffff880007e00000         104M     RW         PSE GLB NX pmd
0xffff880007e00000-0xffff880007ffd000        2036K     RW             GLB NX pte
0xffff880007ffd000-0xffff880008000000          12K                           pte
0xffff880008000000-0xffff880040000000         896M                           pmd
0xffff880040000000-0xffff888000000000         511G                           pud
0xffff888000000000-0xffffc90000000000       66048G                           pgd

---[ vmalloc() Area ]---
[...]
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000 16M pmd

0xffffffff81000000-0xffffffff81400000           4M     ro         PSE GLB x  pmd
0xffffffff81400000-0xffffffff81600000           2M     ro         PSE GLB NX pmd
0xffffffff81600000-0xffffffff81800000           2M     RW         PSE GLB NX pmd

0xffffffff81800000-0xffffffffa0000000 488M pmd
---[ Modules ]---

Signed-off-by Matthieu CASTET <[email protected]>

---
 arch/x86/kernel/head_64.S       |   18 ++++++++++++++++++
 arch/x86/kernel/trampoline_64.S |    4 ++--
 arch/x86/mm/init.c              |    6 ++++--
 arch/x86/mm/init_64.c           |    6 +++++-
 4 files changed, 29 insertions(+), 5 deletions(-)

arch/x86/kernel/head_64.S +18 -0

index 239046b..47f56dc 100644
@@ -139,6 +139,9 @@ ident_complete:
 #ifdef CONFIG_X86_TRAMPOLINE
 	addq	%rbp, trampoline_level4_pgt + 0(%rip)
 	addq	%rbp, trampoline_level4_pgt + (511*8)(%rip)
+
+	addq	%rbp, trampoline_level3_ident_pgt + 0(%rip)
+	addq	%rbp, trampoline_level3_ident_pgt + (L3_START_KERNEL*8)(%rip)
 #endif
 
 	/* Due to ENTRY(), sometimes the empty space gets filled with

@@ -396,6 +399,21 @@ NEXT_PAGE(level2_kernel_pgt)
 NEXT_PAGE(level2_spare_pgt)
 	.fill   512, 8, 0
 
+#ifdef CONFIG_X86_TRAMPOLINE
+NEXT_PAGE(trampoline_level3_ident_pgt)
+	.quad	trampoline_level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.fill	L3_START_KERNEL-1,8,0
+	.quad	trampoline_level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.fill	511-L3_START_KERNEL,8,0
+
+
+NEXT_PAGE(trampoline_level2_ident_pgt)
+	/* Since I easily can, map the first 1G.
+	 * Don't set NX because code runs from these pages.
+	 */
+	PMDS(0, __PAGE_KERNEL_IDENT_LARGE_EXEC, PTRS_PER_PMD)
+#endif
+
 #undef PMDS
 #undef NEXT_PAGE

arch/x86/kernel/trampoline_64.S +2 -2

index 075d130..a408935 100644
@@ -160,8 +160,8 @@ trampoline_stack:
 	.org 0x1000
 trampoline_stack_end:
 ENTRY(trampoline_level4_pgt)
-	.quad	level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.quad	trampoline_level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
 	.fill	510,8,0
-	.quad	level3_kernel_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.quad	trampoline_level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
 
 ENTRY(trampoline_end)

arch/x86/mm/init.c +4 -2

index 947f42a..58d173b 100644
@@ -366,8 +366,10 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 	 * we are going to free part of that, we need to make that
 	 * writeable and non-executable first.
 	 */
-	set_memory_nx(begin, (end - begin) >> PAGE_SHIFT);
-	set_memory_rw(begin, (end - begin) >> PAGE_SHIFT);
+	if (kernel_set_to_readonly) {
+		set_memory_nx(begin, (end - begin) >> PAGE_SHIFT);
+		set_memory_rw(begin, (end - begin) >> PAGE_SHIFT);
+	}
 
 	printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);

arch/x86/mm/init_64.c +5 -1

index 71a5929..3840ecf 100644
@@ -788,6 +788,7 @@ void mark_rodata_ro(void)
 	unsigned long rodata_start =
 		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
+	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
 	unsigned long data_start = (unsigned long) &_sdata;

@@ -798,11 +799,14 @@ void mark_rodata_ro(void)
 
 	kernel_set_to_readonly = 1;
 
+	/* make low level mapping NX */
+	set_memory_nx(PAGE_OFFSET, (PMD_PAGE_SIZE*PTRS_PER_PMD) >> PAGE_SHIFT);
+
 	/*
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();

Signature · 1 line

1.7.2.3

Lin Ming

2011-01-25 12:36 UTC #17 lore raw

Quoted 27 lines · matthieu castet

Quoted 26 lines · matthieu castet
Quoted 10 lines · Lin Ming
Quoted 8 lines · Peter Zijlstra
Quoted 6 lines · Lin Ming
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...
I tried to do the same thing on qemu, and the same behavior happened (ie reboot when resuming cpu1).
After enabling qemu log, I found that a triple fault was happening at the beginning of secondary_startup_64
when doing "addq phys_base(%rip), %rax".
Why ?
I suppose because we access data set to NX, but we don't have enabled yet NX in the msr. So the cpu crash due to "reserved bit check".
If we enable NX before reading data, there is no more crash (patch attached).

Quoted 8 lines

Quoted 7 lines
Now I am not sure this is the correct fix. I think the problem is that trampoline using kernel page table
is very dangerous. The kernel can have modified them atfer booting ! May be all the paging stuff should have been done in head_64.S. A first one with identity mapping, and the second one for
the real kernel stuff.
Lin, could you try this patch on your x64 machine.

Hi,

I'm on holiday now.
I'll test it when I'm back on Feb 9.

Thanks,
Lin Ming

Quoted 6 lines

Thanks
Matthieu

matthieu castet

2011-03-09 23:16 UTC #18 lore raw

In reply to matthieu castet

Hi,

Quoted 17 lines · matthieu castet

Quoted 16 lines · matthieu castet
Quoted 7 lines · Lin Ming
Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Lin Ming
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...
I tried to do the same thing on qemu, and the same behavior happened (ie reboot when resuming cpu1).
After enabling qemu log, I found that a triple fault was happening at the beginning of secondary_startup_64 when doing "addq phys_base(%rip), %rax".
Why ?
I suppose because we access data set to NX, but we don't have enabled yet NX in the msr. So the cpu crash due to "reserved bit check".

Quoted 10 lines

Quoted 7 lines
If we enable NX before reading data, there is no more crash (patch attached).
Now I am not sure this is the correct fix. I think the problem is that trampoline using kernel page table is very dangerous. The kernel can have modified them atfer booting !
May be all the paging stuff should have been done in head_64.S. A first one with identity mapping, and the second one for the real kernel stuff.
Lin, could you try this patch on your x64 machine.

I updated the patch to last tip. I tested on a 64 bits config and everything works.

Any comment on it ?

Thanks

Matthieu

From: Matthieu CASTET <[email protected]>
Date: Thu, 10 Mar 2011 00:10:01 +0100
Subject: [PATCH] x86 : Add NX protection for kernel data on 64 bit

We also set the Low kernel Mapping to NX.

Finaly we apply nx in free_init_pages only when we switch to NX mode in order to preserve large page mapping.

mapping now look like :
---[ Low Kernel Mapping ]---

0xffff880000000000-0xffff880000200000           2M     RW             GLB NX pte
0xffff880000200000-0xffff880001000000          14M     RW         PSE GLB NX pmd
0xffff880001000000-0xffff880001200000           2M     ro         PSE GLB NX pmd
0xffff880001200000-0xffff8800012ae000         696K     ro             GLB NX pte
0xffff8800012ae000-0xffff880001400000        1352K     RW             GLB NX pte
0xffff880001400000-0xffff880001503000        1036K     ro             GLB NX pte
0xffff880001503000-0xffff880001600000        1012K     RW             GLB NX pte
0xffff880001600000-0xffff880007e00000         104M     RW         PSE GLB NX pmd
0xffff880007e00000-0xffff880007ffd000        2036K     RW             GLB NX pte
0xffff880007ffd000-0xffff880008000000          12K                           pte
0xffff880008000000-0xffff880040000000         896M                           pmd
0xffff880040000000-0xffff888000000000         511G                           pud
0xffff888000000000-0xffffc90000000000       66048G                           pgd

---[ vmalloc() Area ]---
[...]
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000 16M pmd

0xffffffff81000000-0xffffffff81400000           4M     ro         PSE GLB x  pmd
0xffffffff81400000-0xffffffff81600000           2M     ro         PSE GLB NX pmd
0xffffffff81600000-0xffffffff81800000           2M     RW         PSE GLB NX pmd

0xffffffff81800000-0xffffffffa0000000 488M pmd
---[ Modules ]---

Signed-off-by Matthieu CASTET <[email protected]>

Conflicts:

	arch/x86/kernel/head_64.S
---
arch/x86/kernel/head_64.S       |   15 +++++++++++++++
arch/x86/kernel/trampoline_64.S |    4 ++--
arch/x86/mm/init.c              |    6 ++++--
arch/x86/mm/init_64.c           |    6 +++++-
4 files changed, 26 insertions(+), 5 deletions(-)

arch/x86/kernel/head_64.S +0 -0

index e11e394..e261354 100644
@@ -140,6 +140,9 @@ ident_complete:

	addq	%rbp, trampoline_level4_pgt + 0(%rip)
	addq	%rbp, trampoline_level4_pgt + (511*8)(%rip)

+ addq %rbp, trampoline_level3_ident_pgt + 0(%rip)
+ addq %rbp, trampoline_level3_ident_pgt + (L3_START_KERNEL*8)(%rip)
+
/* Due to ENTRY(), sometimes the empty space gets filled with
* zeros. Better take a jmp than relying on empty space being
* filled with 0x90 (nop)
@@ -395,6 +398,18 @@ NEXT_PAGE(level2_kernel_pgt) NEXT_PAGE(level2_spare_pgt)

	.fill   512, 8, 0

+NEXT_PAGE(trampoline_level3_ident_pgt)
+ .quad trampoline_level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+ .fill L3_START_KERNEL-1,8,0
+ .quad trampoline_level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+ .fill 511-L3_START_KERNEL,8,0
+
+
+NEXT_PAGE(trampoline_level2_ident_pgt)
+ /* Since I easily can, map the first 1G.
+ * Don't set NX because code runs from these pages.
+ */
+ PMDS(0, __PAGE_KERNEL_IDENT_LARGE_EXEC, PTRS_PER_PMD)
#undef PMDS
#undef NEXT_PAGE

arch/x86/kernel/trampoline_64.S +0 -0

index 09ff517..8723e47 100644
@@ -164,8 +164,8 @@ trampoline_stack:

	.org 0x1000

trampoline_stack_end:
ENTRY(trampoline_level4_pgt)
- .quad level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+ .quad trampoline_level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE

	.fill	510,8,0

- .quad level3_kernel_pgt - __START_KERNEL_map + _KERNPG_TABLE
+ .quad trampoline_level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE

ENTRY(trampoline_end)

arch/x86/mm/init.c +0 -0

index 286d289..98dd5fa 100644
@@ -338,8 +338,10 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)

* we are going to free part of that, we need to make that
* writeable and non-executable first.
*/
- set_memory_nx(begin, (end - begin) >> PAGE_SHIFT);
- set_memory_rw(begin, (end - begin) >> PAGE_SHIFT);
+ if (kernel_set_to_readonly) {
+ set_memory_nx(begin, (end - begin) >> PAGE_SHIFT);
+ set_memory_rw(begin, (end - begin) >> PAGE_SHIFT);
+ }

	printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);

arch/x86/mm/init_64.c +0 -0

index 470cc47..e9bb29d 100644
@@ -832,6 +832,7 @@ void mark_rodata_ro(void)

	unsigned long rodata_start =
		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
+	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
	unsigned long data_start = (unsigned long) &_sdata;

@@ -842,11 +843,14 @@ void mark_rodata_ro(void)

	kernel_set_to_readonly = 1;

+ /* make low level mapping NX */
+ set_memory_nx(PAGE_OFFSET, (PMD_PAGE_SIZE*PTRS_PER_PMD) >> PAGE_SHIFT);
+

	/*
	 * The rodata section (but not the kernel text!) should also be
	 * not-executable.
	 */

- set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+ set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);

	rodata_test();

Signature · 1 line

1.7.4.1

Lin Ming

author

2011-03-10 02:39 UTC #19 lore raw

Quoted 33 lines · matthieu castet

Hi,
Quoted 30 lines · matthieu castet
Quoted 29 lines · matthieu castet
Quoted 7 lines · Lin Ming
Quoted 6 lines · Peter Zijlstra
Quoted 5 lines · Lin Ming
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...
I tried to do the same thing on qemu, and the same behavior happened (ie reboot when resuming cpu1).
After enabling qemu log, I found that a triple fault was happening at the beginning of secondary_startup_64
when doing "addq phys_base(%rip), %rax".
Why ?
I suppose because we access data set to NX, but we don't have enabled yet NX in the msr. So the cpu crash due to "reserved bit check".
If we enable NX before reading data, there is no more crash (patch attached).
Now I am not sure this is the correct fix. I think the problem is that trampoline using kernel page table
is very dangerous. The kernel can have modified them atfer booting ! May be all the paging stuff should have been done in head_64.S. A first one with identity mapping, and the second one for
the real kernel stuff.

Quoted 4 lines

Quoted 3 lines
Lin, could you try this patch on your x64 machine.
I updated the patch to last tip. I tested on a 64 bits config and everything works.

I tested suspend/resume/hotplug and it works on my 64 bit notebook too.

Thanks,
Lin Ming

Quoted 154 lines

Any comment on it ?

Thanks

Matthieu

From: Matthieu CASTET <[email protected]>
Date: Thu, 10 Mar 2011 00:10:01 +0100
Subject: [PATCH] x86 : Add NX protection for kernel data on 64 bit

We also set the Low kernel Mapping to NX.

Finaly we apply nx in free_init_pages only when we switch to NX mode in order to preserve large page mapping.

mapping now look like :
---[ Low Kernel Mapping ]---

0xffff880000000000-0xffff880000200000           2M     RW             GLB NX pte
0xffff880000200000-0xffff880001000000          14M     RW         PSE GLB NX pmd
0xffff880001000000-0xffff880001200000           2M     ro         PSE GLB NX pmd
0xffff880001200000-0xffff8800012ae000         696K     ro             GLB NX pte
0xffff8800012ae000-0xffff880001400000        1352K     RW             GLB NX pte
0xffff880001400000-0xffff880001503000        1036K     ro             GLB NX pte
0xffff880001503000-0xffff880001600000        1012K     RW             GLB NX pte
0xffff880001600000-0xffff880007e00000         104M     RW         PSE GLB NX pmd
0xffff880007e00000-0xffff880007ffd000        2036K     RW             GLB NX pte
0xffff880007ffd000-0xffff880008000000          12K                           pte
0xffff880008000000-0xffff880040000000         896M                           pmd
0xffff880040000000-0xffff888000000000         511G                           pud
0xffff888000000000-0xffffc90000000000       66048G                           pgd

---[ vmalloc() Area ]---
[...]
---[ High Kernel Mapping ]---
0xffffffff80000000-0xffffffff81000000 16M pmd

0xffffffff81000000-0xffffffff81400000           4M     ro         PSE GLB x  pmd
0xffffffff81400000-0xffffffff81600000           2M     ro         PSE GLB NX pmd
0xffffffff81600000-0xffffffff81800000           2M     RW         PSE GLB NX pmd

0xffffffff81800000-0xffffffffa0000000 488M pmd
---[ Modules ]---

Signed-off-by Matthieu CASTET <[email protected]>

Conflicts:

	arch/x86/kernel/head_64.S
---
 arch/x86/kernel/head_64.S       |   15 +++++++++++++++
 arch/x86/kernel/trampoline_64.S |    4 ++--
 arch/x86/mm/init.c              |    6 ++++--
 arch/x86/mm/init_64.c           |    6 +++++-
 4 files changed, 26 insertions(+), 5 deletions(-)

arch/x86/kernel/head_64.S +15 -0

index e11e394..e261354 100644
@@ -140,6 +140,9 @@ ident_complete:
 	addq	%rbp, trampoline_level4_pgt + 0(%rip)
 	addq	%rbp, trampoline_level4_pgt + (511*8)(%rip)
 
+	addq	%rbp, trampoline_level3_ident_pgt + 0(%rip)
+	addq	%rbp, trampoline_level3_ident_pgt + (L3_START_KERNEL*8)(%rip)
+
 	/* Due to ENTRY(), sometimes the empty space gets filled with
 	 * zeros. Better take a jmp than relying on empty space being
 	 * filled with 0x90 (nop)

@@ -395,6 +398,18 @@ NEXT_PAGE(level2_kernel_pgt)
 NEXT_PAGE(level2_spare_pgt)
 	.fill   512, 8, 0
 
+NEXT_PAGE(trampoline_level3_ident_pgt)
+	.quad	trampoline_level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.fill	L3_START_KERNEL-1,8,0
+	.quad	trampoline_level2_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.fill	511-L3_START_KERNEL,8,0
+
+
+NEXT_PAGE(trampoline_level2_ident_pgt)
+	/* Since I easily can, map the first 1G.
+	 * Don't set NX because code runs from these pages.
+	 */
+	PMDS(0, __PAGE_KERNEL_IDENT_LARGE_EXEC, PTRS_PER_PMD)
 #undef PMDS
 #undef NEXT_PAGE

arch/x86/kernel/trampoline_64.S +2 -2

index 09ff517..8723e47 100644
@@ -164,8 +164,8 @@ trampoline_stack:
 	.org 0x1000
 trampoline_stack_end:
 ENTRY(trampoline_level4_pgt)
-	.quad	level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.quad	trampoline_level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
 	.fill	510,8,0
-	.quad	level3_kernel_pgt - __START_KERNEL_map + _KERNPG_TABLE
+	.quad	trampoline_level3_ident_pgt - __START_KERNEL_map + _KERNPG_TABLE
 
 ENTRY(trampoline_end)

arch/x86/mm/init.c +4 -2

index 286d289..98dd5fa 100644
@@ -338,8 +338,10 @@ void free_init_pages(char *what, unsigned long begin, unsigned long end)
 	 * we are going to free part of that, we need to make that
 	 * writeable and non-executable first.
 	 */
-	set_memory_nx(begin, (end - begin) >> PAGE_SHIFT);
-	set_memory_rw(begin, (end - begin) >> PAGE_SHIFT);
+	if (kernel_set_to_readonly) {
+		set_memory_nx(begin, (end - begin) >> PAGE_SHIFT);
+		set_memory_rw(begin, (end - begin) >> PAGE_SHIFT);
+	}
 
 	printk(KERN_INFO "Freeing %s: %luk freed\n", what, (end - begin) >> 10);

arch/x86/mm/init_64.c +5 -1

index 470cc47..e9bb29d 100644
@@ -832,6 +832,7 @@ void mark_rodata_ro(void)
 	unsigned long rodata_start =
 		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
+	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
 	unsigned long data_start = (unsigned long) &_sdata;

@@ -842,11 +843,14 @@ void mark_rodata_ro(void)
 
 	kernel_set_to_readonly = 1;
 
+	/* make low level mapping NX */
+	set_memory_nx(PAGE_OFFSET, (PMD_PAGE_SIZE*PTRS_PER_PMD) >> PAGE_SHIFT);
+
 	/*
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();

matthieu castet

2010-12-24 17:26 UTC #20 lore raw

In reply to Lin Ming

Hi,

Quoted 36 lines · Lin Ming

Quoted 35 lines · mat
Quoted 20 lines · mat
Quoted 6 lines · Andi Kleen
Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
Quoted 7 lines
Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.
yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).
Quoted 6 lines
So they can't end up int the kernel data mapping ?
So it should something else.
I will try to investigate on this.
Unfortunately on my laptop supporting NX, suspend to ram seems broken (even without this patch) and I got only one core, so I am unable to test it.
Does cpu suspend/resume is broken ? Or it is only S3 ?

Quoted 8 lines

Quoted 2 lines
If yes, are there any interesting trace if we suspend only one core with sysfs.
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...

Ok,

could you try the attached patch ?

Thanks

Matthieu

arch/x86/mm/init_64.c +2 -1

index 71a5929..d86552f 100644
@@ -790,6 +790,7 @@ void mark_rodata_ro(void)
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
+	unsigned long data_end = PAGE_ALIGN((unsigned long) &_edata);
 	unsigned long data_start = (unsigned long) &_sdata;
 
 	printk(KERN_INFO "Write protecting the kernel read-only data: %luk\n",

@@ -802,7 +803,7 @@ void mark_rodata_ro(void)
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (data_end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();

Lin Ming

author

2010-12-27 02:06 UTC #21 lore raw

Quoted 44 lines · matthieu castet

Hi,
Quoted 39 lines · Lin Ming
Quoted 37 lines · mat
Quoted 20 lines · mat
Quoted 6 lines · Andi Kleen
Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
Quoted 7 lines
Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.
yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).
Quoted 6 lines
So they can't end up int the kernel data mapping ?
So it should something else.
I will try to investigate on this.
Unfortunately on my laptop supporting NX, suspend to ram seems broken (even without this patch) and I got only one core, so I am unable to test it.
Does cpu suspend/resume is broken ? Or it is only S3 ?
If yes, are there any interesting trace if we suspend only one core with sysfs.

Quoted 8 lines

Quoted 5 lines
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...
Ok,
could you try the attached patch ?

Unfortunately, it does not work against current tip/master(1f7107c8).

Lin Ming

Quoted 4 lines

Thanks
Matthieu

matthieu castet

2011-01-05 18:58 UTC #22 lore raw

Quoted 50 lines · Lin Ming

Quoted 49 lines · matthieu castet
Hi,
Quoted 44 lines · Lin Ming
Quoted 29 lines · mat
Quoted 20 lines · mat
Quoted 6 lines · Andi Kleen
Quoted 6 lines
That seems to be a S3 specific code path, that won't fix anything. Simply do:
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
Quoted 7 lines
Quoted 1 line
and your machine will explode..
The SMP startup trampoline is copied I believe and only executed in real mode without page tables.
So it's perhaps not the trampoline, but the early startup code that ends up being broken.
yes :
acpi wakeup code and smp trampoline are copied in low memory (first 1MB).
Quoted 6 lines
So they can't end up int the kernel data mapping ?
So it should something else.
I will try to investigate on this.
Quoted 8 lines
Unfortunately on my laptop supporting NX, suspend to ram seems broken (even without this patch) and I got only one core, so I am unable to test it.
Does cpu suspend/resume is broken ? Or it is only S3 ?
If yes, are there any interesting trace if we suspend only one core with sysfs.
echo 0 > /sys/devices/system/cpu/cpu1/online;
echo 1 > /sys/devices/system/cpu/cpu1/online;
then machine just reboots...

Quoted 8 lines

Quoted 3 lines
Ok,
could you try the attached patch ?
Unfortunately, it does not work against current tip/master(1f7107c8).
Lin Ming

Thanks for the testing.

For the record I did some tests.
- The x86_32 cpu hotplug is working fine.
- On x86_64 we crash between the cpu1 is wakeup and before trampoline jump to start_secondary

A difference between 32 and 64 bit is that head_32.S use initial_page_table mmu table then switch to swapper_pg_dir while 64 bit version use init_level4_pgt for trampoline, head and kernel.

But I fail to see where setting NX is level3_kernel_pgt aera can make the resume fail. When we enable NX we are in .text section (head_64).

Matthieu

tip-bot for Lin Ming

author

2010-11-22 13:43 UTC #23 lore raw

[tip:x86/security] x86: Resume trampoline must be executable

In reply to Lin Ming

Commit-ID:  691513f70d3957939a318da970987b876c720861
Gitweb:     http://git.kernel.org/tip/691513f70d3957939a318da970987b876c720861
Author:     Lin Ming <[email protected]>
AuthorDate: Mon, 22 Nov 2010 14:03:28 +0100
Committer:  Thomas Gleixner <[email protected]>
CommitDate: Mon, 22 Nov 2010 14:38:52 +0100

x86: Resume trampoline must be executable

commit 5bd5a452(x86: Add NX protection for kernel data) marked the trampoline area NX - which unsurprisingly breaks resume and cpu hotplug.

Revert the portion of that commit, which touches the trampoline.

Originally-from: Lin Ming <[email protected]>
LKML-Reference: <[email protected]>

Cc Matthieu Castet <[email protected]>

Cc Siarhei Liakh <[email protected]>

Cc Xuxian Jiang <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Arjan van de Ven <[email protected]>

Cc Andi Kleen <[email protected]>

Tested-by Peter Zijlstra <[email protected]>

Signed-off-by Thomas Gleixner <[email protected]>

---
 arch/x86/mm/init_64.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

arch/x86/mm/init_64.c +1 -2

index ce59c05..71a5929 100644
@@ -788,7 +788,6 @@ void mark_rodata_ro(void)
 	unsigned long rodata_start =
 		((unsigned long)__start_rodata + PAGE_SIZE - 1) & PAGE_MASK;
 	unsigned long end = (unsigned long) &__end_rodata_hpage_align;
-	unsigned long kernel_end = (((unsigned long)&__init_end + HPAGE_SIZE) & HPAGE_MASK);
 	unsigned long text_end = PAGE_ALIGN((unsigned long) &__stop___ex_table);
 	unsigned long rodata_end = PAGE_ALIGN((unsigned long) &__end_rodata);
 	unsigned long data_start = (unsigned long) &_sdata;

@@ -803,7 +802,7 @@ void mark_rodata_ro(void)
 	 * The rodata section (but not the kernel text!) should also be
 	 * not-executable.
 	 */
-	set_memory_nx(rodata_start, (kernel_end - rodata_start) >> PAGE_SHIFT);
+	set_memory_nx(rodata_start, (end - rodata_start) >> PAGE_SHIFT);
 
 	rodata_test();