2020-04-28 15:27:35

by Joerg Roedel

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

From: Joerg Roedel <[email protected]>

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 <[email protected]>
---
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
--
2.17.1


2020-05-19 09:17:33

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH v3 35/75] x86/head/64: Build k/head64.c with -fno-stack-protector

On Tue, Apr 28, 2020 at 05:16:45PM +0200, Joerg Roedel wrote:
> From: Joerg Roedel <[email protected]>
>
> 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.

--
Regards/Gruss,
Boris.

https://people.kernel.org/tglx/notes-about-netiquette

2020-05-19 14:00:36

by Brian Gerst

[permalink] [raw]
Subject: Re: [PATCH v3 35/75] x86/head/64: Build k/head64.c with -fno-stack-protector

On Tue, Apr 28, 2020 at 11:28 AM Joerg Roedel <[email protected]> wrote:
>
> From: Joerg Roedel <[email protected]>
>
> 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 <[email protected]>
> ---
> 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

2020-06-03 15:21:10

by Joerg Roedel

[permalink] [raw]
Subject: Re: [PATCH v3 35/75] x86/head/64: Build k/head64.c with -fno-stack-protector

On Tue, May 19, 2020 at 09:58:18AM -0400, Brian Gerst wrote:
> On Tue, Apr 28, 2020 at 11:28 AM Joerg Roedel <[email protected]> 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

2020-06-03 15:23:34

by Joerg Roedel

[permalink] [raw]
Subject: Re: [PATCH v3 35/75] x86/head/64: Build k/head64.c with -fno-stack-protector

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 <[email protected]>
> >
> > 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

2020-06-03 17:16:56

by Brian Gerst

[permalink] [raw]
Subject: Re: [PATCH v3 35/75] x86/head/64: Build k/head64.c with -fno-stack-protector

On Wed, Jun 3, 2020 at 11:18 AM Joerg Roedel <[email protected]> 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 <[email protected]> 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