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 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 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
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 > > > > 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
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 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
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 wrote: > > From: Joerg Roedel > > 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 > --- > 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
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 > > 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
[PATCH v3 35/75] x86/head/64: Build k/head64.c with -fno-stack-protector
From: Joerg Roedel 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 --- 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
