| Message ID | 20210820151933.22401-24-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
On Fri, Aug 20, 2021 at 10:19:18AM -0500, Brijesh Singh wrote: > From: Michael Roth <michael.roth@amd.com> > > As of commit 103a4908ad4d ("x86/head/64: Disable stack protection for > head$(BITS).o") kernel/head64.c is compiled with -fno-stack-protector > to allow a call to set_bringup_idt_handler(), which would otherwise > have stack protection enabled with CONFIG_STACKPROTECTOR_STRONG. While > sufficient for that case, this will still cause issues if we attempt to > call out to any external functions that were compiled with stack > protection enabled that in-turn make stack-protected calls, or if the > exception handlers set up by set_bringup_idt_handler() make calls to > stack-protected functions. > > Subsequent patches for SEV-SNP CPUID validation support will introduce > both such cases. Attempting to disable stack protection for everything > in scope to address that is prohibitive since much of the code, like > SEV-ES #VC handler, is shared code that remains in use after boot and > could benefit from having stack protection enabled. Attempting to inline > calls is brittle and can quickly balloon out to library/helper code > where that's not really an option. > > Instead, set up %gs to point a buffer that stack protector can use for > canary values when needed. > > In doing so, it's likely we can stop using -no-stack-protector for > head64.c, but that hasn't been tested yet, and head32.c would need a > similar solution to be safe, so that is left as a potential follow-up. That... > Signed-off-by: Michael Roth <michael.roth@amd.com> > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kernel/Makefile | 2 +- > arch/x86/kernel/head64.c | 20 ++++++++++++++++++++ > 2 files changed, 21 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 3e625c61f008..5abdfd0dbbc3 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -46,7 +46,7 @@ endif > # non-deterministic coverage. > KCOV_INSTRUMENT := n > > -CFLAGS_head$(BITS).o += -fno-stack-protector > +CFLAGS_head32.o += -fno-stack-protector ... and that needs to be taken care of too. > CFLAGS_irq.o := -I $(srctree)/$(src)/../include/asm/trace > > diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c > index a1711c4594fa..f1b76a54c84e 100644 > --- a/arch/x86/kernel/head64.c > +++ b/arch/x86/kernel/head64.c > @@ -74,6 +74,11 @@ static struct desc_struct startup_gdt[GDT_ENTRIES] = { > [GDT_ENTRY_KERNEL_DS] = GDT_ENTRY_INIT(0xc093, 0, 0xfffff), > }; > > +/* For use by stack protector code before switching to virtual addresses */ > +#if CONFIG_STACKPROTECTOR That's "#ifdef". Below too. Did you even build this with CONFIG_STACKPROTECTOR disabled? Because if you did, you would've seen this: arch/x86/kernel/head64.c:78:5: warning: "CONFIG_STACKPROTECTOR" is not defined, evaluates to 0 [-Wundef] 78 | #if CONFIG_STACKPROTECTOR | ^~~~~~~~~~~~~~~~~~~~~ arch/x86/kernel/head64.c: In function ‘startup_64_setup_env’: arch/x86/kernel/head64.c:613:35: error: ‘startup_gs_area’ undeclared (first use in this function) 613 | u64 gs_area = (u64)fixup_pointer(startup_gs_area, physbase); | ^~~~~~~~~~~~~~~ arch/x86/kernel/head64.c:613:35: note: each undeclared identifier is reported only once for each function it appears in arch/x86/kernel/head64.c:632:5: warning: "CONFIG_STACKPROTECTOR" is not defined, evaluates to 0 [-Wundef] 632 | #if CONFIG_STACKPROTECTOR | ^~~~~~~~~~~~~~~~~~~~~ arch/x86/kernel/head64.c:613:6: warning: unused variable ‘gs_area’ [-Wunused-variable] 613 | u64 gs_area = (u64)fixup_pointer(startup_gs_area, physbase); | ^~~~~~~ make[2]: *** [scripts/Makefile.build:271: arch/x86/kernel/head64.o] Error 1 make[1]: *** [scripts/Makefile.build:514: arch/x86/kernel] Error 2 make[1]: *** Waiting for unfinished jobs.... make: *** [Makefile:1851: arch/x86] Error 2 make: *** Waiting for unfinished jobs.... > +static char startup_gs_area[64]; > +#endif > + > /* > * Address needs to be set at runtime because it references the startup_gdt > * while the kernel still uses a direct mapping. > @@ -605,6 +610,8 @@ void early_setup_idt(void) > */ > void __head startup_64_setup_env(unsigned long physbase) > { > + u64 gs_area = (u64)fixup_pointer(startup_gs_area, physbase); > + > /* Load GDT */ > startup_gdt_descr.address = (unsigned long)fixup_pointer(startup_gdt, physbase); > native_load_gdt(&startup_gdt_descr); > @@ -614,5 +621,18 @@ void __head startup_64_setup_env(unsigned long physbase) > "movl %%eax, %%ss\n" > "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory"); > > + /* > + * GCC stack protection needs a place to store canary values. The > + * default is %gs:0x28, which is what the kernel currently uses. > + * Point GS base to a buffer that can be used for this purpose. > + * Note that newer GCCs now allow this location to be configured, > + * so if we change from the default in the future we need to ensure > + * that this buffer overlaps whatever address ends up being used. > + */ > +#if CONFIG_STACKPROTECTOR > + asm volatile("movl %%eax, %%gs\n" : : "a"(__KERNEL_DS) : "memory"); > + native_wrmsr(MSR_GS_BASE, gs_area, gs_area >> 32); > +#endif > + > startup_64_load_idt(physbase); > } > -- > 2.17.1 >
On Fri, Aug 20, 2021 at 10:19:18AM -0500, Brijesh Singh wrote: > void __head startup_64_setup_env(unsigned long physbase) > { > + u64 gs_area = (u64)fixup_pointer(startup_gs_area, physbase); > + This breaks as soon as the compiler decides that startup_64_setup_env() needs stack protection too. And the startup_gs_area is also not needed, there is initial_gs for that. What you need is something along these lines (untested): diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S index d8b3ebd2bb85..3c7c59bc9903 100644 --- a/arch/x86/kernel/head_64.S +++ b/arch/x86/kernel/head_64.S @@ -65,6 +65,16 @@ SYM_CODE_START_NOALIGN(startup_64) leaq (__end_init_task - FRAME_SIZE)(%rip), %rsp leaq _text(%rip), %rdi + + movl $MSR_GS_BASE, %ecx + movq initial_gs(%rip), %rax + movq $_text, %rdx + subq %rdx, %rax + addq %rdi, %rax + movq %rax, %rdx + shrq $32, %rdx + wrmsr + pushq %rsi call startup_64_setup_env popq %rsi It loads the initial_gs pointer, applies the fixup on it and loads it into MSR_GS_BASE.
On Wed, Aug 25, 2021 at 04:29:13PM +0200, Borislav Petkov wrote: > On Fri, Aug 20, 2021 at 10:19:18AM -0500, Brijesh Singh wrote: > > From: Michael Roth <michael.roth@amd.com> > > > > As of commit 103a4908ad4d ("x86/head/64: Disable stack protection for > > head$(BITS).o") kernel/head64.c is compiled with -fno-stack-protector > > to allow a call to set_bringup_idt_handler(), which would otherwise > > have stack protection enabled with CONFIG_STACKPROTECTOR_STRONG. While > > sufficient for that case, this will still cause issues if we attempt to > > call out to any external functions that were compiled with stack > > protection enabled that in-turn make stack-protected calls, or if the > > exception handlers set up by set_bringup_idt_handler() make calls to > > stack-protected functions. > > > > Subsequent patches for SEV-SNP CPUID validation support will introduce > > both such cases. Attempting to disable stack protection for everything > > in scope to address that is prohibitive since much of the code, like > > SEV-ES #VC handler, is shared code that remains in use after boot and > > could benefit from having stack protection enabled. Attempting to inline > > calls is brittle and can quickly balloon out to library/helper code > > where that's not really an option. > > > > Instead, set up %gs to point a buffer that stack protector can use for > > canary values when needed. > > > > In doing so, it's likely we can stop using -no-stack-protector for > > head64.c, but that hasn't been tested yet, and head32.c would need a > > similar solution to be safe, so that is left as a potential follow-up. > > That... Argh! I had this fixed up but I think it got clobbered in the patch shuffle. I'll make sure to fix this, and remember to actually test without CONFIG_STACKPTROTECTOR this time. Sorry for the screw-up. > > > Signed-off-by: Michael Roth <michael.roth@amd.com> > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > > --- > > arch/x86/kernel/Makefile | 2 +- > > arch/x86/kernel/head64.c | 20 ++++++++++++++++++++ > > 2 files changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > > index 3e625c61f008..5abdfd0dbbc3 100644 > > --- a/arch/x86/kernel/Makefile > > +++ b/arch/x86/kernel/Makefile > > @@ -46,7 +46,7 @@ endif > > # non-deterministic coverage. > > KCOV_INSTRUMENT := n > > > > -CFLAGS_head$(BITS).o += -fno-stack-protector > > +CFLAGS_head32.o += -fno-stack-protector > > ... and that needs to be taken care of too. I didn't realize the the 32-bit path was something you were suggesting to have added in this patch, but I'll take a look at that as well.
On Wed, Aug 25, 2021 at 10:18:35AM -0500, Michael Roth wrote: > On Wed, Aug 25, 2021 at 04:29:13PM +0200, Borislav Petkov wrote: > > On Fri, Aug 20, 2021 at 10:19:18AM -0500, Brijesh Singh wrote: > > > From: Michael Roth <michael.roth@amd.com> > > > > > > As of commit 103a4908ad4d ("x86/head/64: Disable stack protection for > > > head$(BITS).o") kernel/head64.c is compiled with -fno-stack-protector > > > to allow a call to set_bringup_idt_handler(), which would otherwise > > > have stack protection enabled with CONFIG_STACKPROTECTOR_STRONG. While > > > sufficient for that case, this will still cause issues if we attempt to ^^^ I'm tired of repeating the same review comments with you guys: Who's "we"? Please use passive voice in your text: no "we" or "I", etc. Personal pronouns are ambiguous in text, especially with so many parties/companies/etc developing the kernel so let's avoid them please. How about you pay more attention? > I didn't realize the the 32-bit path was something you were suggesting > to have added in this patch, but I'll take a look at that as well. If you're going to remove the -no-stack-protector thing for that file, then pls remove it for both 32- and 64-bit. I.e., the revert what 103a4908ad4d did.
On Wed, Aug 25, 2021 at 05:07:26PM +0200, Joerg Roedel wrote: > On Fri, Aug 20, 2021 at 10:19:18AM -0500, Brijesh Singh wrote: > > void __head startup_64_setup_env(unsigned long physbase) > > { > > + u64 gs_area = (u64)fixup_pointer(startup_gs_area, physbase); > > + > > This breaks as soon as the compiler decides that startup_64_setup_env() > needs stack protection too. Good point. > > And the startup_gs_area is also not needed, there is initial_gs for > that. > > What you need is something along these lines (untested): > > diff --git a/arch/x86/kernel/head_64.S b/arch/x86/kernel/head_64.S > index d8b3ebd2bb85..3c7c59bc9903 100644 > --- a/arch/x86/kernel/head_64.S > +++ b/arch/x86/kernel/head_64.S > @@ -65,6 +65,16 @@ SYM_CODE_START_NOALIGN(startup_64) > leaq (__end_init_task - FRAME_SIZE)(%rip), %rsp > > leaq _text(%rip), %rdi > + > + movl $MSR_GS_BASE, %ecx > + movq initial_gs(%rip), %rax > + movq $_text, %rdx > + subq %rdx, %rax > + addq %rdi, %rax > + movq %rax, %rdx > + shrq $32, %rdx > + wrmsr > + > pushq %rsi > call startup_64_setup_env > popq %rsi > > > It loads the initial_gs pointer, applies the fixup on it and loads it > into MSR_GS_BASE. This seems to do the trick, and is probably closer to what the 32-bit version would look like. Thanks for the suggestion!
On Wed, Aug 25, 2021 at 06:29:31PM +0200, Borislav Petkov wrote: > On Wed, Aug 25, 2021 at 10:18:35AM -0500, Michael Roth wrote: > > On Wed, Aug 25, 2021 at 04:29:13PM +0200, Borislav Petkov wrote: > > > On Fri, Aug 20, 2021 at 10:19:18AM -0500, Brijesh Singh wrote: > > > > From: Michael Roth <michael.roth@amd.com> > > > > > > > > As of commit 103a4908ad4d ("x86/head/64: Disable stack protection for > > > > head$(BITS).o") kernel/head64.c is compiled with -fno-stack-protector > > > > to allow a call to set_bringup_idt_handler(), which would otherwise > > > > have stack protection enabled with CONFIG_STACKPROTECTOR_STRONG. While > > > > sufficient for that case, this will still cause issues if we attempt to > ^^^ > > I'm tired of repeating the same review comments with you guys: > > Who's "we"? > > Please use passive voice in your text: no "we" or "I", etc. > Personal pronouns are ambiguous in text, especially with so many > parties/companies/etc developing the kernel so let's avoid them please. That had also been fixed in the commit message fixup that got clobbered, but I still missed it in one of the comments as well so I'll be more careful of this. > > How about you pay more attention? I've been periodically revising/rewording my comments since I saw you're original comments to Brijesh a few versions back, but it's how I normally talk when discussing code with people so it keeps managing to sneak back in. I've added a git hook to check for this and found other instances that need fixing as well, so hopefully with the help of technology I can get them all sorted for the next spin. > > > I didn't realize the the 32-bit path was something you were suggesting > > to have added in this patch, but I'll take a look at that as well. > > If you're going to remove the -no-stack-protector thing for that file, > then pls remove it for both 32- and 64-bit. I.e., the revert what > 103a4908ad4d did. Got it, will do. > > -- > Regards/Gruss, > Boris. > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpeople.kernel.org%2Ftglx%2Fnotes-about-netiquette&data=04%7C01%7Cmichael.roth%40amd.com%7Cdfceeb76d2a4481da83f08d967e57220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637655057436180426%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=aAKQ%2B7mXBvL4oofk0y7CacaMMD8Ucg8YL5hB4nw7zgo%3D&reserved=0
On Fri, Aug 27, 2021 at 08:38:31AM -0500, Michael Roth wrote: > I've been periodically revising/rewording my comments since I saw you're > original comments to Brijesh a few versions back, but it's how I normally > talk when discussing code with people so it keeps managing to sneak back in. Oh sure, happens to me too and I know it is hard to keep out but when you start doing git archeology and start going through old commit messages, wondering why stuff was done the way it is sitting there, you'd be very grateful if someone actually took the time to write up the "why" properly. Why was it done this way, what the constraints were, yadda yadda. And when you see a "we" there, you sometimes wonder, who's "we"? Was it the party who submitted the code, was it the person who's submitting the code but talking with the generic voice of a programmer who means "we" the community writing the kernel, etc. So yes, it is ambiguous and it probably wasn't a big deal at all when the people writing the kernel all knew each other back then but that long ain't the case anymore. So we (see, snuck in on me too :)) ... so maintainers need to pay attention to those things now too. Oh look, the last "we" above meant "maintainers". I believe that should explain with a greater detail what I mean. :-) > I've added a git hook to check for this and found other instances that need > fixing as well, so hopefully with the help of technology I can get them all > sorted for the next spin. Thanks, very much appreciated!
On Tue, Aug 31, 2021 at 10:03:12AM +0200, Borislav Petkov wrote: > On Fri, Aug 27, 2021 at 08:38:31AM -0500, Michael Roth wrote: > > I've been periodically revising/rewording my comments since I saw you're > > original comments to Brijesh a few versions back, but it's how I normally > > talk when discussing code with people so it keeps managing to sneak back in. > > Oh sure, happens to me too and I know it is hard to keep out but when > you start doing git archeology and start going through old commit > messages, wondering why stuff was done the way it is sitting there, > you'd be very grateful if someone actually took the time to write up the > "why" properly. Why was it done this way, what the constraints were, > yadda yadda. > > And when you see a "we" there, you sometimes wonder, who's "we"? Was it > the party who submitted the code, was it the person who's submitting the > code but talking with the generic voice of a programmer who means "we" > the community writing the kernel, etc. > > So yes, it is ambiguous and it probably wasn't a big deal at all when > the people writing the kernel all knew each other back then but that > long ain't the case anymore. So we (see, snuck in on me too :)) ... so > maintainers need to pay attention to those things now too. > > Oh look, the last "we" above meant "maintainers". > > I believe that should explain with a greater detail what I mean. > > :-) Thanks for the explanation, makes perfect sense. Just need to get my brain on the same page. :)
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile index 3e625c61f008..5abdfd0dbbc3 100644 --- a/arch/x86/kernel/Makefile +++ b/arch/x86/kernel/Makefile @@ -46,7 +46,7 @@ endif # non-deterministic coverage. KCOV_INSTRUMENT := n -CFLAGS_head$(BITS).o += -fno-stack-protector +CFLAGS_head32.o += -fno-stack-protector CFLAGS_irq.o := -I $(srctree)/$(src)/../include/asm/trace diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c index a1711c4594fa..f1b76a54c84e 100644 --- a/arch/x86/kernel/head64.c +++ b/arch/x86/kernel/head64.c @@ -74,6 +74,11 @@ static struct desc_struct startup_gdt[GDT_ENTRIES] = { [GDT_ENTRY_KERNEL_DS] = GDT_ENTRY_INIT(0xc093, 0, 0xfffff), }; +/* For use by stack protector code before switching to virtual addresses */ +#if CONFIG_STACKPROTECTOR +static char startup_gs_area[64]; +#endif + /* * Address needs to be set at runtime because it references the startup_gdt * while the kernel still uses a direct mapping. @@ -605,6 +610,8 @@ void early_setup_idt(void) */ void __head startup_64_setup_env(unsigned long physbase) { + u64 gs_area = (u64)fixup_pointer(startup_gs_area, physbase); + /* Load GDT */ startup_gdt_descr.address = (unsigned long)fixup_pointer(startup_gdt, physbase); native_load_gdt(&startup_gdt_descr); @@ -614,5 +621,18 @@ void __head startup_64_setup_env(unsigned long physbase) "movl %%eax, %%ss\n" "movl %%eax, %%es\n" : : "a"(__KERNEL_DS) : "memory"); + /* + * GCC stack protection needs a place to store canary values. The + * default is %gs:0x28, which is what the kernel currently uses. + * Point GS base to a buffer that can be used for this purpose. + * Note that newer GCCs now allow this location to be configured, + * so if we change from the default in the future we need to ensure + * that this buffer overlaps whatever address ends up being used. + */ +#if CONFIG_STACKPROTECTOR + asm volatile("movl %%eax, %%gs\n" : : "a"(__KERNEL_DS) : "memory"); + native_wrmsr(MSR_GS_BASE, gs_area, gs_area >> 32); +#endif + startup_64_load_idt(physbase); }