[v3,35/75] x86/head/64: Build k/head64.c with -fno-stack-protector

Commit Message

Joerg Roedel April 28, 2020, 3:16 p.m. UTC

From: Joerg Roedel <[e-mail]>

The code inserted by the stack protector does not work in the early
boot environment because it uses the GS segment, at least with memory
encryption enabled. Make sure the early code is compiled without this
feature enabled.

Signed-off-by: Joerg Roedel <[e-mail]>
---
 arch/x86/kernel/Makefile | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Borislav Petkov May 19, 2020, 9:15 a.m. UTC | #1

On Tue, Apr 28, 2020 at 05:16:45PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[e-mail]>
> 
> The code inserted by the stack protector does not work in the early
> boot environment because it uses the GS segment, at least with memory
> encryption enabled.

Can you elaborate on why is that a problem?

The stack cookie is not generated that early yet so it should be
comparing %gs:40 to 0.

Also, it generates the checking code here only with

CONFIG_STACKPROTECTOR_STRONG=y

> Make sure the early code is compiled without this feature enabled.

If so, then this should be with CONFIG_AMD_MEM_ENCRYPT ifdeffery around
it.

Brian Gerst May 19, 2020, 1:58 p.m. UTC | #2

On Tue, Apr 28, 2020 at 11:28 AM Joerg Roedel <[e-mail]> wrote:
>
> From: Joerg Roedel <[e-mail]>
>
> The code inserted by the stack protector does not work in the early
> boot environment because it uses the GS segment, at least with memory
> encryption enabled. Make sure the early code is compiled without this
> feature enabled.
>
> Signed-off-by: Joerg Roedel <[e-mail]>
> ---
>  arch/x86/kernel/Makefile | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index ba89cabe5fcf..1192de38fa56 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -35,6 +35,10 @@ ifdef CONFIG_FRAME_POINTER
>  OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o             := y
>  endif
>
> +# make sure head64.c is built without stack protector
> +nostackp := $(call cc-option, -fno-stack-protector)
> +CFLAGS_head64.o                := $(nostackp)
> +
>  # If instrumentation of this dir is enabled, boot hangs during first second.
>  # Probably could be more selective here, but note that files related to irqs,
>  # boot, dumpstack/stacktrace, etc are either non-interesting or can lead to

The proper fix would be to initialize MSR_GS_BASE earlier.

--
Brian Gerst

Joerg Roedel June 3, 2020, 3:18 p.m. UTC | #3

On Tue, May 19, 2020 at 09:58:18AM -0400, Brian Gerst wrote:
> On Tue, Apr 28, 2020 at 11:28 AM Joerg Roedel <[e-mail]> wrote:

> The proper fix would be to initialize MSR_GS_BASE earlier.

That'll mean to initialize it two times during boot, as the first C
function with stack-protection is called before the kernel switches to
its high addresses (early_idt_setup call-path). But okay, I can do that.

On the other side, which value does the stack protector have in the early
boot code?


	Joerg

Joerg Roedel June 3, 2020, 3:21 p.m. UTC | #4

On Tue, May 19, 2020 at 11:15:26AM +0200, Borislav Petkov wrote:
> On Tue, Apr 28, 2020 at 05:16:45PM +0200, Joerg Roedel wrote:
> > From: Joerg Roedel <[e-mail]>
> > 
> > The code inserted by the stack protector does not work in the early
> > boot environment because it uses the GS segment, at least with memory
> > encryption enabled.
> 
> Can you elaborate on why is that a problem?
> 
> The stack cookie is not generated that early yet so it should be
> comparing %gs:40 to 0.

Yes, and when GS_BASE is 0 it will dereference NULL pointer, which
generates a page-fault before the kernel is able to handle it.


	Joerg

Brian Gerst June 3, 2020, 5:14 p.m. UTC | #5

On Wed, Jun 3, 2020 at 11:18 AM Joerg Roedel <[e-mail]> wrote:
>
> On Tue, May 19, 2020 at 09:58:18AM -0400, Brian Gerst wrote:
> > On Tue, Apr 28, 2020 at 11:28 AM Joerg Roedel <[e-mail]> wrote:
>
> > The proper fix would be to initialize MSR_GS_BASE earlier.
>
> That'll mean to initialize it two times during boot, as the first C
> function with stack-protection is called before the kernel switches to
> its high addresses (early_idt_setup call-path). But okay, I can do that.

Good point.  Since this is boot code which isn't subject to stack
smashing attacks, disabling stack protector is probably the simpler
option.

--
Brian Gerst

Patch

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index ba89cabe5fcf..1192de38fa56 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -35,6 +35,10 @@  ifdef CONFIG_FRAME_POINTER
 OBJECT_FILES_NON_STANDARD_ftrace_$(BITS).o		:= y
 endif
 
+# make sure head64.c is built without stack protector
+nostackp := $(call cc-option, -fno-stack-protector)
+CFLAGS_head64.o		:= $(nostackp)
+
 # If instrumentation of this dir is enabled, boot hangs during first second.
 # Probably could be more selective here, but note that files related to irqs,
 # boot, dumpstack/stacktrace, etc are either non-interesting or can lead to