x86: Make sure verify_cpu has a good stack

RFC PATCH In review Borislav Petkov 5 participants 34 messages Mar 2, 2016 – Mar 4, 2016

Subsystems (2)

x86 Generic include/asm header files

lore

Share & tools

lore

Borislav Petkov

author maintainer

2016-03-02 11:20 UTC #0 lore raw

RFC PATCH 1/1 x86: Make sure verify_cpu has a good stack

From: Borislav Petkov <[email protected]>

04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too") added the call to verify_cpu() for sanitizing CPU configuration.

The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.

Do that.

APs don't need this as the trampoline sets up a stack for them.

Reported-by Tom Lendacky <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 arch/x86/kernel/head_64.S | 4 ++++
 1 file changed, 4 insertions(+)

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

index 22fbf9df61bb..d60a044c2fdc 100644
@@ -64,6 +64,10 @@ startup_64:
 	 * tables and then reload them.
 	 */
 
+	/* Setup a stack for verify_cpu */
+	movq    stack_start - __START_KERNEL_map, %rsp
+	subq	$__START_KERNEL_map, %rsp
+
 	/* Sanitize CPU configuration */
 	call verify_cpu

Signature · 1 line

2.3.5

Mika Penttilä

2016-03-02 16:10 UTC #1 lore raw

Quoted 24 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too") added the call to verify_cpu() for sanitizing CPU configuration.
The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.
Do that.
APs don't need this as the trampoline sets up a stack for them.
Reported-by Tom Lendacky <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/head_64.S | 4 ++++
 1 file changed, 4 insertions(+)
arch/x86/kernel/head_64.S +0 -0
index 22fbf9df61bb..d60a044c2fdc 100644

arch/x86/kernel/head_64.S startup_64: · line 70

Quoted 7 lines

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

@@ -64,6 +64,10 @@ startup_64:
 	 * tables and then reload them.
 	 */
 
+	/* Setup a stack for verify_cpu */
+	movq    stack_start - __START_KERNEL_map, %rsp
+	subq	$__START_KERNEL_map, %rsp
+

You subtract __START_KERNEL_map twice ?

--Mika

Borislav Petkov

author maintainer

2016-03-02 16:15 UTC #2 lore raw

Quoted 6 lines · Mika Penttilä

arch/x86/kernel/head_64.S startup_64: · line 70
Quoted 4 lines
arch/x86/kernel/head_64.S +4 -0
+	/* Setup a stack for verify_cpu */
+	movq    stack_start - __START_KERNEL_map, %rsp
+	subq	$__START_KERNEL_map, %rsp
+
You subtract __START_KERNEL_map twice ?

Yes. That's not very obvious and it took me a while. I probably should add a comment.

Want to stare at it a little bit more and try to figure it out or should I explain?

:-)

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Mika Penttilä

2016-03-02 16:38 UTC #3 lore raw

Quoted 6 lines · Borislav Petkov

Quoted 5 lines · Mika Penttilä
arch/x86/kernel/head_64.S startup_64: · line 70
Quoted 4 lines
arch/x86/kernel/head_64.S +4 -0
+	/* Setup a stack for verify_cpu */
+	movq    stack_start - __START_KERNEL_map, %rsp
+	subq	$__START_KERNEL_map, %rsp
+
You subtract __START_KERNEL_map twice ?

Quoted 8 lines

Yes. That's not very obvious and it took me a while. I probably should add a comment.
Want to stare at it a little bit more and try to figure it out or should I explain?
:-)

I actually looked at it a while too...

The
movq stack_start - __START_KERNEL_map, %rsp

turns into (objdump disassembly)

mov 0x0,%rsp

with relocation
0000000000000004 R_X86_64_32S stack_start+0x0000000080000000

Now stack_start is at ffffffff81ef3380, so the relocation gives 1ef3380 which would be correct, so why the second subq ?

You may explain :)

--Mika

Borislav Petkov

author maintainer

2016-03-02 16:55 UTC #4 lore raw

Quoted 9 lines · Mika Penttilä

I actually looked at it a while too...
The
movq stack_start - __START_KERNEL_map, %rsp
turns into (objdump disassembly)
mov 0x0,%rsp

Quoted 7 lines

with relocation
0000000000000004 R_X86_64_32S stack_start+0x0000000080000000
Now stack_start is at ffffffff81ef3380, so the relocation gives 1ef3380 which would be correct, so why the second subq ?
You may explain :)

Here it is :-)

$ readelf -a vmlinux | grep stack_start
 70526: ffffffff81cbabf8     0 NOTYPE  GLOBAL DEFAULT   14 stack_start

0xffffffff81cbabf8 - __START_KERNEL_map =
0xffffffff81cbabf8 - 0xffffffff80000000 =
0x1cbabf8

(gdb) x/x 0x1cbabf8
0x1cbabf8: 0xffffffff81c03ff8

(You don't need gdb for that - you can hexdump or objdump vmlinux).

Now stack_start is:

GLOBAL(stack_start)
.quad init_thread_union+THREAD_SIZE-8

which is

$ readelf -a vmlinux | grep init_thread_union
 82491: ffffffff81c00000 16384 OBJECT  GLOBAL DEFAULT   14 init_thread_union

so init_thread_union+THREAD_SIZE-8 = 0xffffffff81c00000 + 4*4096-8 = 0xffffffff81c03ff8

So you have to subtract __START_KERNEL_map again because it has there a virtual address and we haven't enabled paging yet:

0xffffffff81c03ff8 - 0xffffffff80000000 = 0x1c03ff8.

Makes sense?

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Mika Penttilä

2016-03-02 17:44 UTC #5 lore raw

Quoted 41 lines · Borislav Petkov

Quoted 9 lines · Mika Penttilä
I actually looked at it a while too...
The
movq stack_start - __START_KERNEL_map, %rsp
turns into (objdump disassembly)
mov 0x0,%rsp
Quoted 7 lines
with relocation
0000000000000004 R_X86_64_32S stack_start+0x0000000080000000
Now stack_start is at ffffffff81ef3380, so the relocation gives 1ef3380 which would be correct, so why the second subq ?
You may explain :)
Here it is :-)
$ readelf -a vmlinux | grep stack_start
 70526: ffffffff81cbabf8     0 NOTYPE  GLOBAL DEFAULT   14 stack_start
0xffffffff81cbabf8 - __START_KERNEL_map =
0xffffffff81cbabf8 - 0xffffffff80000000 =
0x1cbabf8
(gdb) x/x 0x1cbabf8
0x1cbabf8: 0xffffffff81c03ff8
(You don't need gdb for that - you can hexdump or objdump vmlinux).
Now stack_start is:
GLOBAL(stack_start)
.quad init_thread_union+THREAD_SIZE-8
which is
$ readelf -a vmlinux | grep init_thread_union
 82491: ffffffff81c00000 16384 OBJECT  GLOBAL DEFAULT   14 init_thread_union

Quoted 9 lines

so init_thread_union+THREAD_SIZE-8 = 0xffffffff81c00000 + 4*4096-8 = 0xffffffff81c03ff8
So you have to subtract __START_KERNEL_map again because it has there a virtual address and we haven't enabled paging yet:
0xffffffff81c03ff8 - 0xffffffff80000000 = 0x1c03ff8.
Makes sense?

Ah missed completely that stack_start is effectively a pointer to stack..

Thanks,
Mika

Brian Gerst

2016-03-02 16:22 UTC #6 lore raw

In reply to Borislav Petkov

Quoted 24 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too") added the call to verify_cpu() for sanitizing CPU configuration.
The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.
Do that.
APs don't need this as the trampoline sets up a stack for them.
Reported-by Tom Lendacky <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
---
 arch/x86/kernel/head_64.S | 4 ++++
 1 file changed, 4 insertions(+)
arch/x86/kernel/head_64.S +0 -0
index 22fbf9df61bb..d60a044c2fdc 100644

arch/x86/kernel/head_64.S startup_64: · line 68

Quoted 5 lines

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

@@ -64,6 +64,10 @@ startup_64:
         * tables and then reload them.
         */
 
+       /* Setup a stack for verify_cpu */
+       movq    stack_start - __START_KERNEL_map, %rsp

This should be: movq stack_start(%rip), %rsp

arch/x86/kernel/head_64.S startup_64: · line 69

Quoted 1 line

arch/x86/kernel/head_64.S +1 -0
+       subq    $__START_KERNEL_map, %rsp

It would be better to add the offset to the initializer for stack_start instead of adjusting it at runtime. That would require moving the existing load of stack_start from the common path to the secondary startup, which probably isn't a bad thing as it wouldn't depend on the trampoline stack anymore.

--
Brian Gerst

Borislav Petkov

author maintainer

2016-03-02 16:25 UTC #7 lore raw

Quoted 1 line · Brian Gerst

This should be: movq stack_start(%rip), %rsp

No it wouldn't. That doesn't work.

Quoted 6 lines

Quoted 1 line
+ subq $__START_KERNEL_map, %rsp
It would be better to add the offset to the initializer for stack_start instead of adjusting it at runtime. That would require moving the existing load of stack_start from the common path to the secondary startup,

This is the BSP we're talking about - no secondary startup. We want to run verify_cpu as early as possible.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-02 17:54 UTC #8 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 1 line · Brian Gerst
This should be: movq stack_start(%rip), %rsp
No it wouldn't. That doesn't work.
Quoted 2 lines
Quoted 1 line
+ subq $__START_KERNEL_map, %rsp

Quoted 8 lines

Quoted 4 lines
It would be better to add the offset to the initializer for stack_start instead of adjusting it at runtime. That would require moving the existing load of stack_start from the common path to the secondary startup,
This is the BSP we're talking about - no secondary startup. We want to run
verify_cpu as early as possible.

Please explain why we can't use rip-relative addressing in some form...

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Borislav Petkov

author maintainer

2016-03-02 18:16 UTC #9 lore raw

Quoted 1 line · H. Peter Anvin

Please explain why we can't use rip-relative addressing in some form...

We *can* do almost what Brian suggested:

movq stack_start(%rip), %rsp
subq $__START_KERNEL_map, %rsp

But we still have to subtract __START_KERNEL_map.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-02 18:25 UTC #10 lore raw

Quoted 10 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin
Please explain why we can't use rip-relative addressing in some
form...
We *can* do almost what Brian suggested:
movq stack_start(%rip), %rsp
subq $__START_KERNEL_map, %rsp
But we still have to subtract __START_KERNEL_map.

Obviously.

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

H. Peter Anvin

reviewer

2016-03-02 18:39 UTC #11 lore raw

In reply to Borislav Petkov

Quoted 10 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin
Please explain why we can't use rip-relative addressing in some form...
We *can* do almost what Brian suggested:
movq stack_start(%rip), %rsp
subq $__START_KERNEL_map, %rsp
But we still have to subtract __START_KERNEL_map.

Well, we definitely should use %rip-relative addressing if we can.

However, even so I believe this breaks if the kernel is loaded anywhere but its default load address. I think we need to do something like:

	movq	stack_start(%rip), %rax
	leaq	__START_KERNEL_map(%rip), %rdx
	subq	%rdx, %rax
	movq	%rax, %rsp

The use of temporary registers avoids clobbering a valid stack pointer for even a single instruction if we are given one.

-hpa

Borislav Petkov

author maintainer

2016-03-02 19:51 UTC #12 lore raw

Quoted 1 line · H. Peter Anvin

Well, we definitely should use %rip-relative addressing if we can.

Right you are.

Quoted 10 lines

However, even so I believe this breaks if the kernel is loaded anywhere but its default load address. I think we need to do something like:
	movq	stack_start(%rip), %rax
	leaq	__START_KERNEL_map(%rip), %rdx
	subq	%rdx, %rax
	movq	%rax, %rsp
The use of temporary registers avoids clobbering a valid stack pointer for even a single instruction if we are given one.

Yeah, we should be prudent and make this as sturdy as possible. I did this:

CONFIG_PHYSICAL_START=0x100beef

and it aligned startup_64 up to ffffffff82000000. It seems to boot fine in kvm. But better safe than sorry.

Thanks.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Borislav Petkov

author maintainer

2016-03-02 20:46 UTC #13 lore raw

Quoted 1 line · Borislav Petkov

But better safe than sorry.

I got this, it looks good when I'm single-stepping through it with gdb and it boots fine in kvm. I'll run it on baremetal tomorrow:

/*
* Setup stack for verify_cpu(): make sure we don't clobber a valid
* stack pointer by using temporary registers.
*/
movq stack_start(%rip), %rax
movq $__START_KERNEL_map, %rdx
subq %rdx, %rax
movq %rax, %rsp

Thanks.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-02 21:35 UTC #14 lore raw

In reply to Borislav Petkov

Quoted 16 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin
Well, we definitely should use %rip-relative addressing if we can.
Right you are.
Quoted 10 lines
However, even so I believe this breaks if the kernel is loaded anywhere but its default load address. I think we need to do something like:
	movq	stack_start(%rip), %rax
	leaq	__START_KERNEL_map(%rip), %rdx
	subq	%rdx, %rax
	movq	%rax, %rsp
The use of temporary registers avoids clobbering a valid stack pointer for even a single instruction if we are given one.

Quoted 7 lines

Yeah, we should be prudent and make this as sturdy as possible. I did this:
CONFIG_PHYSICAL_START=0x100beef
and it aligned startup_64 up to ffffffff82000000. It seems to boot fine in kvm. But better safe than sorry.

You're not actually testing anything as the real issue is what happens with a relocating bootloader. That's okay; I think we can be pretty sure the above works by inspection.

-hpa

Borislav Petkov

author maintainer

2016-03-02 21:47 UTC #15 lore raw

Quoted 2 lines · H. Peter Anvin

You're not actually testing anything as the real issue is what happens with a relocating bootloader.

Hmm, how would that relocation happen so that va - __START_KERNEL_map doesn't give pa?

Or do you mean something else with "relocating bootloader"? Do you know of one which does that?

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-02 21:59 UTC #16 lore raw

Quoted 10 lines · Borislav Petkov

Quoted 2 lines · H. Peter Anvin
You're not actually testing anything as the real issue is what happens with a relocating bootloader.
Hmm, how would that relocation happen so that va - __START_KERNEL_map doesn't give pa?
Or do you mean something else with "relocating bootloader"? Do you know of one which does that?

A relocating bootloader is one that doesn't load the kernel at CONFIG_PHYSICAL_ADDRESS. The EFI stub is one example.

__START_KERNEL_map is not relocated. On x86-64 we do relocation by pointing the page tables at a different address.

So I really think we need this to be a leaq, so we take a nonstandard load address into consideration.

-hpa

Borislav Petkov

author maintainer

2016-03-02 22:10 UTC #17 lore raw

Quoted 8 lines · H. Peter Anvin

A relocating bootloader is one that doesn't load the kernel at CONFIG_PHYSICAL_ADDRESS. The EFI stub is one example.
__START_KERNEL_map is not relocated. On x86-64 we do relocation by pointing the page tables at a different address.
So I really think we need this to be a leaq, so we take a nonstandard load address into consideration.

Hmm, but __START_KERNEL_map is a simple macro:

#define __START_KERNEL_map _AC(0xffffffff80000000, UL)

Ok, I think you want to do something like this for stack_start too:

/*
* Compute the delta between the address I am compiled to run at and the
* address I am actually running at.
*/
leaq _text(%rip), %rbp
subq $_text - __START_KERNEL_map, %rbp
...

in the normal case %rbp is 0, of course.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-02 22:12 UTC #18 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 6 lines · H. Peter Anvin
A relocating bootloader is one that doesn't load the kernel at CONFIG_PHYSICAL_ADDRESS. The EFI stub is one example.
__START_KERNEL_map is not relocated. On x86-64 we do relocation by pointing the page tables at a different address.

Quoted 6 lines

Quoted 2 lines
So I really think we need this to be a leaq, so we take a nonstandard load address into consideration.
Hmm, but __START_KERNEL_map is a simple macro:
#define __START_KERNEL_map _AC(0xffffffff80000000, UL)

That should not be a problem.

Quoted 11 lines

Ok, I think you want to do something like this for stack_start too:
/*
* Compute the delta between the address I am compiled to run at and the
* address I am actually running at.
*/
leaq _text(%rip), %rbp
subq $_text - __START_KERNEL_map, %rbp
...

Quoted 2 lines

in the normal case %rbp is 0, of course.

Not sure if we need a reference to _text here.

-hpa

Borislav Petkov

author maintainer

2016-03-02 22:28 UTC #19 lore raw

Quoted 1 line · H. Peter Anvin

Not sure if we need a reference to _text here.

Ah, so stack_start is in .ref.data, I guess we can add a __ref_data marker in vmlinux.lds.S to denote the start of the .ref.data section and use that for calculating the delta...

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-02 22:33 UTC #20 lore raw

Quoted 7 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin
Not sure if we need a reference to _text here.
Ah, so stack_start is in .ref.data, I guess we can add a __ref_data marker in vmlinux.lds.S to denote the start of the .ref.data section and
use that for calculating the delta...

I'm trying to think of any reason why we couldn't simply have a symbol at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Borislav Petkov

author maintainer

2016-03-02 22:40 UTC #21 lore raw

Quoted 3 lines · H. Peter Anvin

I'm trying to think of any reason why we couldn't simply have a symbol at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.

That should be simpler. And we do games like that already in the trampoline:

# Setup stack
movl $rm_stack_end, %esp

...

GLOBAL(rm_stack)

        .space  2048

GLOBAL(rm_stack_end)

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Yinghai Lu

2016-03-03 00:13 UTC #22 lore raw

In reply to H. Peter Anvin

Quoted 2 lines · H. Peter Anvin

I'm trying to think of any reason why we couldn't simply have a symbol at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.

Why do we need to call verify_cpu in arch/x86/kernel/head_64.S aka the vmlinux again ?

Is that already called in arch/x86/boot/compressed/head_64.S?

Or Tom is using 64bit bootloader that use vmlinux instead of bzImage?

Thanks

Yinghai

Yinghai Lu

2016-03-03 01:00 UTC #23 lore raw

Quoted 8 lines · Yinghai Lu

Quoted 2 lines · H. Peter Anvin
I'm trying to think of any reason why we couldn't simply have a symbol at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.
Why do we need to call verify_cpu in arch/x86/kernel/head_64.S aka the vmlinux again ?
Is that already called in arch/x86/boot/compressed/head_64.S?

that calling is from startup_32, so may add another calling in startup_64, so can avoid calling from arch/x86/kernel/head_64.S

Quoted 2 lines

Or Tom is using 64bit bootloader that use vmlinux instead of bzImage?

Yinghai Lu

2016-03-03 02:50 UTC #24 lore raw

Quoted 5 lines · Yinghai Lu

Quoted 4 lines · Yinghai Lu
Quoted 2 lines · H. Peter Anvin
I'm trying to think of any reason why we couldn't simply have a symbol at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.

Quoted 8 lines

Quoted 4 lines
Why do we need to call verify_cpu in arch/x86/kernel/head_64.S aka the vmlinux again ?
Is that already called in arch/x86/boot/compressed/head_64.S?
that calling is from startup_32, so may add another calling in startup_64, so can avoid calling from arch/x86/kernel/head_64.S

at the same time, the "call verify_cpu" in arch/x86/kernel/head_64.s::secondary_startup_64()
is not needed.
As APs already go through arch/x86/realmode/rm/trampoline_64.S::trampoline_start(), and it already
call verify_cpu in 16bit mode.

Thanks

Yinghai

Borislav Petkov

author maintainer

2016-03-03 12:28 UTC #25 lore raw

In reply to H. Peter Anvin

Quoted 3 lines · H. Peter Anvin

I'm trying to think of any reason why we couldn't simply have a symbol at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.

How about something like this:

---
From: Borislav Petkov <[email protected]>
Date: Sun, 28 Feb 2016 21:35:44 +0100
Subject: [PATCH -v2] x86/asm: Make sure verify_cpu() has a good stack
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too") added the call to verify_cpu() for sanitizing CPU configuration.

The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.

Do that.

APs don't need this as the trampoline sets up a stack for them.

Reported-by Tom Lendacky <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc "H. Peter Anvin" <[email protected]>

Cc Mika Penttilä <[email protected]>

---
 arch/x86/kernel/head_64.S         | 3 +++
 include/asm-generic/vmlinux.lds.h | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)

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

index 22fbf9df61bb..968d6408b887 100644
@@ -64,6 +64,9 @@ startup_64:
 	 * tables and then reload them.
 	 */
 
+	/* Setup stack for verify_cpu(). */
+	leaq	(__end_init_task - 8)(%rip), %rsp
+
 	/* Sanitize CPU configuration */
 	call verify_cpu

include/asm-generic/vmlinux.lds.h +3 -1

index 772c784ba763..cba2a26628fc 100644
@@ -246,7 +246,9 @@
 
 #define INIT_TASK_DATA(align)						\
 	. = ALIGN(align);						\
-	*(.data..init_task)
+	VMLINUX_SYMBOL(__start_init_task) = .;				\
+	*(.data..init_task)						\
+	VMLINUX_SYMBOL(__end_init_task) = .;
 
 /*
  * Read only Data

Signature · 7 lines

2.3.5

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-03 15:26 UTC #26 lore raw

Quoted 58 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin
I'm trying to think of any reason why we couldn't simply have a
symbol
Quoted 2 lines
at the top of the initial stack? Then a simple leaq would suffice; this is for the BSP after all.
How about something like this:
---
From: Borislav Petkov <[email protected]>
Date: Sun, 28 Feb 2016 21:35:44 +0100
Subject: [PATCH -v2] x86/asm: Make sure verify_cpu() has a good stack
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too")
added the call to verify_cpu() for sanitizing CPU configuration.
The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.
Do that.
APs don't need this as the trampoline sets up a stack for them.
Reported-by Tom Lendacky <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Mika Penttilä <[email protected]>
---
arch/x86/kernel/head_64.S | 3 +++
include/asm-generic/vmlinux.lds.h | 4 +++-
2 files changed, 6 insertions(+), 1 deletion(-)
arch/x86/kernel/head_64.S +3 -0
index 22fbf9df61bb..968d6408b887 100644
@@ -64,6 +64,9 @@ startup_64:
 	 * tables and then reload them.
 	 */
 
+	/* Setup stack for verify_cpu(). */
+	leaq	(__end_init_task - 8)(%rip), %rsp
+
 	/* Sanitize CPU configuration */
 	call verify_cpu
 
include/asm-generic/vmlinux.lds.h +0 -0
index 772c784ba763..cba2a26628fc 100644

include/asm-generic/vmlinux.lds.h line 254

Quoted 10 lines

include/asm-generic/vmlinux.lds.h +3 -1

@@ -246,7 +246,9 @@
 
 #define INIT_TASK_DATA(align)						\
 	. = ALIGN(align);						\
-	*(.data..init_task)
+	VMLINUX_SYMBOL(__start_init_task) = .;				\
+	*(.data..init_task)						\
+	VMLINUX_SYMBOL(__end_init_task) = .;
 
 /*
 * Read only Data

Why -8?

Signature · 1 line

Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.

Borislav Petkov

author maintainer

2016-03-03 16:30 UTC #27 lore raw

Quoted 1 line · H. Peter Anvin

Why -8?

        GLOBAL(stack_start)
        .quad  init_thread_union+THREAD_SIZE-8
					    ^^^

But I don't see why it needed the -8 then. It came with a conglomerate dump in 2002:

commit af53c7a2c81399b805b6d4eff887401a5e50feef

Author: Andi Kleen <[email protected]>
Date:   Fri Apr 19 20:23:17 2002 -0700

[PATCH] x86-64 architecture specific sync for 2.5.8

- /* Setup the first kernel stack (this instruction is modified by smpboot) */
- .byte 0x48, 0xb8 /* movq *init_rsp,%rax */
-init_rsp:
- .quad init_thread_union+THREAD_SIZE
- movq %rax, %rsp

...

-
- /* SMP bootup changes this */
+ /* SMP bootup changes these two */
.globl initial_code
initial_code:

        .quad   x86_64_start_kernel

+ .globl init_rsp
+init_rsp:
+ .quad init_thread_union+THREAD_SIZE-8
+
---

But since we decrement first and then copy to stack ptr when we push, I don't see why we need the -8.

Do you have a better clue?

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-03 20:22 UTC #28 lore raw

Quoted 37 lines · Borislav Petkov

Quoted 1 line · H. Peter Anvin

Why -8?

        GLOBAL(stack_start)
        .quad  init_thread_union+THREAD_SIZE-8
					    ^^^

But I don't see why it needed the -8 then. It came with a conglomerate dump in 2002:

commit af53c7a2c81399b805b6d4eff887401a5e50feef

Author: Andi Kleen <[email protected]>
Date:   Fri Apr 19 20:23:17 2002 -0700

[PATCH] x86-64 architecture specific sync for 2.5.8

(quoted diff) +5 -8

-       /* Setup the first kernel stack (this instruction is modified by smpboot) */
-       .byte 0x48, 0xb8        /* movq *init_rsp,%rax */ 
-init_rsp:
-       .quad init_thread_union+THREAD_SIZE
-       movq    %rax, %rsp
 
...
 
-
-       /* SMP bootup changes this */   
+       /* SMP bootup changes these two */      
        .globl  initial_code
 initial_code:
        .quad   x86_64_start_kernel
+       .globl init_rsp
+init_rsp:
+       .quad  init_thread_union+THREAD_SIZE-8
+
---

Quoted 5 lines

But since we decrement first and then copy to stack ptr when we push, I don't see why we need the -8.
Do you have a better clue?

The only thing I can think of is that the -8 creates a null pointer that terminates a stack trace.

-hpa

Borislav Petkov

author maintainer

2016-03-03 20:54 UTC #29 lore raw

Quoted 2 lines · H. Peter Anvin

The only thing I can think of is that the -8 creates a null pointer that terminates a stack trace.

Probably not needed anymore as print_context_stack()->valid_stack_ptr() in dumpstack.c look at the stack boundaries instead of checking for NULL...

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

H. Peter Anvin

reviewer

2016-03-03 21:23 UTC #30 lore raw

Quoted 8 lines · Borislav Petkov

Quoted 2 lines · H. Peter Anvin
The only thing I can think of is that the -8 creates a null pointer that terminates a stack trace.
Probably not needed anymore as print_context_stack()->valid_stack_ptr() in dumpstack.c look at the stack boundaries instead of checking for NULL...

Sure, but it could still affect kgdb or what not. That's the only reason I can see for -8 though.

-hpa

Borislav Petkov

author maintainer

2016-03-03 21:38 UTC #31 lore raw

Quoted 1 line · H. Peter Anvin

reason I can see for -8 though.

... and keeping the -8 won't hurt us or anything. I'll add a small comment about it.

Thanks.

Signature · 4 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.

Yinghai Lu

2016-03-04 01:18 UTC #32 lore raw

In reply to Borislav Petkov

Quoted 11 lines · Borislav Petkov

04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too") added the call to verify_cpu() for sanitizing CPU configuration.
The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.
Do that.
APs don't need this as the trampoline sets up a stack for them.

Even more than that. For AP verify_cpu already get called in trampoline.

arch/x86/realmode/rm/trampoline_64.S::trampoline_start().

So you remove verify_cpu calling in secondary_startup_64.

Yinghai

Yinghai Lu

2016-03-04 02:25 UTC #33 lore raw

In reply to Borislav Petkov

Quoted 51 lines · Borislav Petkov

From: Borislav Petkov <[email protected]>
Date: Sun, 28 Feb 2016 21:35:44 +0100
Subject: [PATCH -v2] x86/asm: Make sure verify_cpu() has a good stack
04633df0c43d ("x86/cpu: Call verify_cpu() after having entered long mode too") added the call to verify_cpu() for sanitizing CPU configuration.
The latter uses the stack minimally and it can happen that we land in startup_64() directly from a 64-bit bootloader. Then we want to use our own, known good stack.
Reported-by Tom Lendacky <[email protected]>
Signed-off-by Borislav Petkov <[email protected]>
Cc Brian Gerst <[email protected]>
Cc "H. Peter Anvin" <[email protected]>
Cc Mika Penttilä <[email protected]>
---
 arch/x86/kernel/head_64.S         | 3 +++
 include/asm-generic/vmlinux.lds.h | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)
arch/x86/kernel/head_64.S +3 -0
index 22fbf9df61bb..968d6408b887 100644
@@ -64,6 +64,9 @@ startup_64:
         * tables and then reload them.
         */
 
+       /* Setup stack for verify_cpu(). */
+       leaq    (__end_init_task - 8)(%rip), %rsp
+
        /* Sanitize CPU configuration */
        call verify_cpu
 
include/asm-generic/vmlinux.lds.h +3 -2
index 772c784ba763..cba2a26628fc 100644
@@ -246,7 +246,9 @@
 
 #define INIT_TASK_DATA(align)                                          \
        . = ALIGN(align);                                               \
-       *(.data..init_task)
+       VMLINUX_SYMBOL(__start_init_task) = .;                          \
+       *(.data..init_task)                                             \
+       VMLINUX_SYMBOL(__end_init_task) = .;
 
 /*
  * Read only Data
--

Quoted 1 line

2.3.5

I would suggest moving down verify_cpu calling after offset is calcuated, like following instead of adding __end_init_task.

arch/x86/kernel/head_64.S +8 -3

index 22fbf9d..e8c8085 100644
@@ -64,9 +64,6 @@ startup_64:
         * tables and then reload them.
         */
 
-       /* Sanitize CPU configuration */
-       call verify_cpu
-
        /*
         * Compute the delta between the address I am compiled to run at and the
         * address I am actually running at.

@@ -74,6 +71,14 @@ startup_64:
        leaq    _text(%rip), %rbp
        subq    $_text - __START_KERNEL_map, %rbp
 
+       /* Setup stack for verify_cpu() */
+       movq    stack_start(%rip), %rsp
+       subq    $__START_KERNEL_map, %rsp
+       addq    %rbp, %rsp
+
+       /* Sanitize CPU configuration */
+       call verify_cpu
+
        /* Is the address not 2M aligned? */
        testl   $~PMD_PAGE_MASK, %ebp
        jnz     bad_address