diff mbox series

[Part1,v5,23/38] x86/head/64: set up a startup %gs for stack protector

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

Commit Message

Brijesh Singh Aug. 20, 2021, 3:19 p.m. UTC
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.

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(-)

Comments

Borislav Petkov Aug. 25, 2021, 2:29 p.m. UTC | #1
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
>
Joerg Roedel Aug. 25, 2021, 3:07 p.m. UTC | #2
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.
Michael Roth Aug. 25, 2021, 3:18 p.m. UTC | #3
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.
Borislav Petkov Aug. 25, 2021, 4:29 p.m. UTC | #4
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.
Michael Roth Aug. 25, 2021, 5:07 p.m. UTC | #5
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!
Michael Roth Aug. 27, 2021, 1:38 p.m. UTC | #6
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&amp;data=04%7C01%7Cmichael.roth%40amd.com%7Cdfceeb76d2a4481da83f08d967e57220%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637655057436180426%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=aAKQ%2B7mXBvL4oofk0y7CacaMMD8Ucg8YL5hB4nw7zgo%3D&amp;reserved=0
Borislav Petkov Aug. 31, 2021, 8:03 a.m. UTC | #7
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!
Michael Roth Aug. 31, 2021, 11:30 p.m. UTC | #8
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 mbox series

Patch

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);
 }