2016-12-14 03:27:19

by Josh Poimboeuf

[permalink] [raw]
Subject: [PATCH 0/2] x86/boot/64: a couple of start_cpu() cleanups

Josh Poimboeuf (2):
x86/boot/64: use 'push' instead of 'call' in start_cpu()
x86/boot/64: push correct start_cpu() return address

arch/x86/kernel/head_64.S | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

--
2.7.4


2016-12-14 03:27:17

by Josh Poimboeuf

[permalink] [raw]
Subject: [PATCH 1/2] x86/boot/64: use 'push' instead of 'call' in start_cpu()

start_cpu() pushes a text address on the stack so that stack traces from
idle tasks will show start_cpu() at the end. But it uses a call
instruction to do that, which is rather obtuse. Use a straightforward
push instead.

Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/kernel/head_64.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 90de288..1facaf4 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -298,7 +298,7 @@ ENTRY(start_cpu)
* REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
* address given in m16:64.
*/
- call 1f # put return address on stack for unwinder
+ pushq $1f # put return address on stack for unwinder
1: xorq %rbp, %rbp # clear frame pointer
movq initial_code(%rip), %rax
pushq $__KERNEL_CS # set correct cs
--
2.7.4

2016-12-14 03:27:18

by Josh Poimboeuf

[permalink] [raw]
Subject: [PATCH 2/2] x86/boot/64: push correct start_cpu() return address

start_cpu() pushes a text address on the stack so that stack traces from
idle tasks will show start_cpu() at the end. But it currently shows the
wrong function offset. It's more correct to show the address
immediately after the 'lretq' instruction.

Signed-off-by: Josh Poimboeuf <[email protected]>
---
arch/x86/kernel/head_64.S | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 1facaf4..b467b14 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -298,12 +298,13 @@ ENTRY(start_cpu)
* REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
* address given in m16:64.
*/
- pushq $1f # put return address on stack for unwinder
-1: xorq %rbp, %rbp # clear frame pointer
+ pushq $.Lafter_lret # put return address on stack for unwinder
+ xorq %rbp, %rbp # clear frame pointer
movq initial_code(%rip), %rax
pushq $__KERNEL_CS # set correct cs
pushq %rax # target address in negative space
lretq
+.Lafter_lret:
ENDPROC(start_cpu)

#include "verify_cpu.S"
--
2.7.4

Subject: [tip:x86/urgent] x86/boot/64: Use 'push' instead of 'call' in start_cpu()

Commit-ID: ec2d86a9b646d93f1948569f368e2c6f5449e6c7
Gitweb: http://git.kernel.org/tip/ec2d86a9b646d93f1948569f368e2c6f5449e6c7
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Tue, 13 Dec 2016 21:25:35 -0600
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 14 Dec 2016 08:48:05 +0100

x86/boot/64: Use 'push' instead of 'call' in start_cpu()

start_cpu() pushes a text address on the stack so that stack traces from
idle tasks will show start_cpu() at the end. But it uses a call
instruction to do that, which is rather obtuse. Use a straightforward
push instead.

Suggested-by: Borislav Petkov <[email protected]>
Signed-off-by: Josh Poimboeuf <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/4d8a1952759721d42d1e62ba9e4a7e3ac5df8574.1481685203.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/head_64.S | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 90de288..1facaf4 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -298,7 +298,7 @@ ENTRY(start_cpu)
* REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
* address given in m16:64.
*/
- call 1f # put return address on stack for unwinder
+ pushq $1f # put return address on stack for unwinder
1: xorq %rbp, %rbp # clear frame pointer
movq initial_code(%rip), %rax
pushq $__KERNEL_CS # set correct cs

Subject: [tip:x86/urgent] x86/boot/64: Push correct start_cpu() return address

Commit-ID: 31dcfec11f827e9a5d8720fe4728f1305894884f
Gitweb: http://git.kernel.org/tip/31dcfec11f827e9a5d8720fe4728f1305894884f
Author: Josh Poimboeuf <[email protected]>
AuthorDate: Tue, 13 Dec 2016 21:25:36 -0600
Committer: Ingo Molnar <[email protected]>
CommitDate: Wed, 14 Dec 2016 08:48:05 +0100

x86/boot/64: Push correct start_cpu() return address

start_cpu() pushes a text address on the stack so that stack traces from
idle tasks will show start_cpu() at the end. But it currently shows the
wrong function offset. It's more correct to show the address
immediately after the 'lretq' instruction.

Signed-off-by: Josh Poimboeuf <[email protected]>
Cc: Andy Lutomirski <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Brian Gerst <[email protected]>
Cc: Denys Vlasenko <[email protected]>
Cc: H. Peter Anvin <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Link: http://lkml.kernel.org/r/2cadd9f16c77da7ee7957bfc5e1c67928c23ca48.1481685203.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/x86/kernel/head_64.S | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
index 1facaf4..b467b14 100644
--- a/arch/x86/kernel/head_64.S
+++ b/arch/x86/kernel/head_64.S
@@ -298,12 +298,13 @@ ENTRY(start_cpu)
* REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
* address given in m16:64.
*/
- pushq $1f # put return address on stack for unwinder
-1: xorq %rbp, %rbp # clear frame pointer
+ pushq $.Lafter_lret # put return address on stack for unwinder
+ xorq %rbp, %rbp # clear frame pointer
movq initial_code(%rip), %rax
pushq $__KERNEL_CS # set correct cs
pushq %rax # target address in negative space
lretq
+.Lafter_lret:
ENDPROC(start_cpu)

#include "verify_cpu.S"

2016-12-14 19:27:05

by H. Peter Anvin

[permalink] [raw]
Subject: Re: [tip:x86/urgent] x86/boot/64: Use 'push' instead of 'call' in start_cpu()

On December 14, 2016 12:36:58 AM PST, tip-bot for Josh Poimboeuf <[email protected]> wrote:
>Commit-ID: ec2d86a9b646d93f1948569f368e2c6f5449e6c7
>Gitweb:
>http://git.kernel.org/tip/ec2d86a9b646d93f1948569f368e2c6f5449e6c7
>Author: Josh Poimboeuf <[email protected]>
>AuthorDate: Tue, 13 Dec 2016 21:25:35 -0600
>Committer: Ingo Molnar <[email protected]>
>CommitDate: Wed, 14 Dec 2016 08:48:05 +0100
>
>x86/boot/64: Use 'push' instead of 'call' in start_cpu()
>
>start_cpu() pushes a text address on the stack so that stack traces
>from
>idle tasks will show start_cpu() at the end. But it uses a call
>instruction to do that, which is rather obtuse. Use a straightforward
>push instead.
>
>Suggested-by: Borislav Petkov <[email protected]>
>Signed-off-by: Josh Poimboeuf <[email protected]>
>Cc: Andy Lutomirski <[email protected]>
>Cc: Brian Gerst <[email protected]>
>Cc: Denys Vlasenko <[email protected]>
>Cc: H. Peter Anvin <[email protected]>
>Cc: Linus Torvalds <[email protected]>
>Cc: Peter Zijlstra <[email protected]>
>Cc: Thomas Gleixner <[email protected]>
>Link:
>http://lkml.kernel.org/r/4d8a1952759721d42d1e62ba9e4a7e3ac5df8574.1481685203.git.jpoimboe@redhat.com
>Signed-off-by: Ingo Molnar <[email protected]>
>---
> arch/x86/kernel/head_64.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
>index 90de288..1facaf4 100644
>--- a/arch/x86/kernel/head_64.S
>+++ b/arch/x86/kernel/head_64.S
>@@ -298,7 +298,7 @@ ENTRY(start_cpu)
> * REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
> * address given in m16:64.
> */
>- call 1f # put return address on stack for unwinder
>+ pushq $1f # put return address on stack for unwinder
> 1: xorq %rbp, %rbp # clear frame pointer
> movq initial_code(%rip), %rax
> pushq $__KERNEL_CS # set correct cs

This adds another relocation to the kernel. I hope this is safe at this point in the code?
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

2016-12-14 20:13:24

by Josh Poimboeuf

[permalink] [raw]
Subject: Re: [tip:x86/urgent] x86/boot/64: Use 'push' instead of 'call' in start_cpu()

On Wed, Dec 14, 2016 at 11:24:19AM -0800, [email protected] wrote:
> On December 14, 2016 12:36:58 AM PST, tip-bot for Josh Poimboeuf <[email protected]> wrote:
> >diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S
> >index 90de288..1facaf4 100644
> >--- a/arch/x86/kernel/head_64.S
> >+++ b/arch/x86/kernel/head_64.S
> >@@ -298,7 +298,7 @@ ENTRY(start_cpu)
> > * REX.W + FF /5 JMP m16:64 Jump far, absolute indirect,
> > * address given in m16:64.
> > */
> >- call 1f # put return address on stack for unwinder
> >+ pushq $1f # put return address on stack for unwinder
> > 1: xorq %rbp, %rbp # clear frame pointer
> > movq initial_code(%rip), %rax
> > pushq $__KERNEL_CS # set correct cs
>
> This adds another relocation to the kernel. I hope this is safe at this point in the code?

AFAIK, it should be fine. All relocations were either applied at build
time, or for KASLR, in the compressed boot code which extracts and
copies this code.

Also there are already a bunch of relocations in the rest of the code in
this file, all of which runs before this code does.

(And even if that weren't the case, this address is only used for
displaying stack traces, so pushing a zero or some garbage here wouldn't
really break anything.)

--
Josh