[PATCH v1] x86: Pin cr4 FSGSBASE

[PATCH v1] x86: Pin cr4 FSGSBASE

[Andi Kleen]

From: Andi Kleen <ak@linux.intel.com>

Since there seem to be kernel modules floating around that set
FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
CR4 pinning just checks that bits are set, this also checks
that the FSGSBASE bit is not set, and if it is clears it again.

Note this patch will need to be undone when the full FSGSBASE
patches are merged. But it's a reasonable solution for v5.2+
stable at least. Sadly the older kernels don't have the necessary
infrastructure for this (although a simpler version of this
could be added there too)

Cc: stable@vger.kernel.org # v5.2+
Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 arch/x86/kernel/cpu/common.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index bed0cb83fe24..1f5b7871ae9a 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -385,6 +385,11 @@ void native_write_cr4(unsigned long val)
 		/* Warn after we've set the missing bits. */
 		WARN_ONCE(bits_missing, "CR4 bits went missing: %lx!?\n",
 			  bits_missing);
+		if (val & X86_CR4_FSGSBASE) {
+			WARN_ONCE(1, "CR4 unexpectedly set FSGSBASE!?\n");
+			val &= ~X86_CR4_FSGSBASE;
+			goto set_register;
+		}
 	}
 }
 EXPORT_SYMBOL(native_write_cr4);
-- 
2.25.4

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Greg KH]

On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Since there seem to be kernel modules floating around that set
> FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> CR4 pinning just checks that bits are set, this also checks
> that the FSGSBASE bit is not set, and if it is clears it again.

So we are trying to "protect" ourselves from broken out-of-tree kernel
modules now?  Why stop with this type of check, why not just forbid them
entirely if we don't trust them?  :)

> Note this patch will need to be undone when the full FSGSBASE
> patches are merged. But it's a reasonable solution for v5.2+
> stable at least. Sadly the older kernels don't have the necessary
> infrastructure for this (although a simpler version of this
> could be added there too)
> 
> Cc: stable@vger.kernel.org # v5.2+
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/common.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index bed0cb83fe24..1f5b7871ae9a 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -385,6 +385,11 @@ void native_write_cr4(unsigned long val)
>  		/* Warn after we've set the missing bits. */
>  		WARN_ONCE(bits_missing, "CR4 bits went missing: %lx!?\n",
>  			  bits_missing);
> +		if (val & X86_CR4_FSGSBASE) {
> +			WARN_ONCE(1, "CR4 unexpectedly set FSGSBASE!?\n");

Like this will actually be noticed by anyone who calls this?  What is a
user supposed to do about this?

What about those systems that panic-on-warn?

> +			val &= ~X86_CR4_FSGSBASE;

So you just prevented them from setting this, thereby fixing up their
broken code that will never be fixed because you did this?  Why do this?

thanks,

greg k-h

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Peter Zijlstra]

On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Since there seem to be kernel modules floating around that set
> > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > CR4 pinning just checks that bits are set, this also checks
> > that the FSGSBASE bit is not set, and if it is clears it again.
> 
> So we are trying to "protect" ourselves from broken out-of-tree kernel
> modules now?  Why stop with this type of check, why not just forbid them
> entirely if we don't trust them?  :)

Oh, I have a bunch of patches pending for that :-)

It will basically decode the module text and refuse to load the module
for most CPL0 instruction.

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Greg KH]

On Tue, May 26, 2020 at 09:57:36AM +0200, Peter Zijlstra wrote:
> On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > Since there seem to be kernel modules floating around that set
> > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > CR4 pinning just checks that bits are set, this also checks
> > > that the FSGSBASE bit is not set, and if it is clears it again.
> > 
> > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > modules now?  Why stop with this type of check, why not just forbid them
> > entirely if we don't trust them?  :)
> 
> Oh, I have a bunch of patches pending for that :-)

Ah, I thought I had seen something like that go by a while ago.

It's sad that we have to write a "don't do stupid things" checker for
kernel modules now :(

> It will basically decode the module text and refuse to load the module
> for most CPL0 instruction.

Ok, so why would Andi's patch even be needed then?  Andi, why post this?

thanks,

greg k-h

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Peter Zijlstra]

On Tue, May 26, 2020 at 10:17:52AM +0200, Greg KH wrote:
> On Tue, May 26, 2020 at 09:57:36AM +0200, Peter Zijlstra wrote:
> > On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > Since there seem to be kernel modules floating around that set
> > > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > > CR4 pinning just checks that bits are set, this also checks
> > > > that the FSGSBASE bit is not set, and if it is clears it again.
> > > 
> > > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > > modules now?  Why stop with this type of check, why not just forbid them
> > > entirely if we don't trust them?  :)
> > 
> > Oh, I have a bunch of patches pending for that :-)
> 
> Ah, I thought I had seen something like that go by a while ago.
> 
> It's sad that we have to write a "don't do stupid things" checker for
> kernel modules now :(

Because people... they get stuff from the interweb and run it :/ The
days that admins actually knew what they're doing is long long gone.

> > It will basically decode the module text and refuse to load the module
> > for most CPL0 instruction.
> 
> Ok, so why would Andi's patch even be needed then?  Andi, why post this?

Andi's patch cures a particularly bad module that floats around that
people use, probably without being aware that it's an insta-root hole.

My patches will be a while (too many things in the fire :/) and will
certainly not be for stable.

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Greg KH]

On Tue, May 26, 2020 at 11:17:45AM +0200, Peter Zijlstra wrote:
> On Tue, May 26, 2020 at 10:17:52AM +0200, Greg KH wrote:
> > On Tue, May 26, 2020 at 09:57:36AM +0200, Peter Zijlstra wrote:
> > > On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > > > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > 
> > > > > Since there seem to be kernel modules floating around that set
> > > > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > > > CR4 pinning just checks that bits are set, this also checks
> > > > > that the FSGSBASE bit is not set, and if it is clears it again.
> > > > 
> > > > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > > > modules now?  Why stop with this type of check, why not just forbid them
> > > > entirely if we don't trust them?  :)
> > > 
> > > Oh, I have a bunch of patches pending for that :-)
> > 
> > Ah, I thought I had seen something like that go by a while ago.
> > 
> > It's sad that we have to write a "don't do stupid things" checker for
> > kernel modules now :(
> 
> Because people... they get stuff from the interweb and run it :/ The
> days that admins actually knew what they're doing is long long gone.

{sigh}

> > > It will basically decode the module text and refuse to load the module
> > > for most CPL0 instruction.
> > 
> > Ok, so why would Andi's patch even be needed then?  Andi, why post this?
> 
> Andi's patch cures a particularly bad module that floats around that
> people use, probably without being aware that it's an insta-root hole.

Ok, fair enough, thanks for the context.

greg k-h

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Andi Kleen]

On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Since there seem to be kernel modules floating around that set
> > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > CR4 pinning just checks that bits are set, this also checks
> > that the FSGSBASE bit is not set, and if it is clears it again.
> 
> So we are trying to "protect" ourselves from broken out-of-tree kernel
> modules now?  

Well it's a specific case where we know they're opening a root hole
unintentionally. This is just an pragmatic attempt to protect the users in the 
short term.

> Why stop with this type of check, why not just forbid them
> entirely if we don't trust them?  :)

Would be pointless -- lots of people rely on them, so such a rule
wouldn't survive very long in production kernels.

> > diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> > index bed0cb83fe24..1f5b7871ae9a 100644
> > --- a/arch/x86/kernel/cpu/common.c
> > +++ b/arch/x86/kernel/cpu/common.c
> > @@ -385,6 +385,11 @@ void native_write_cr4(unsigned long val)
> >  		/* Warn after we've set the missing bits. */
> >  		WARN_ONCE(bits_missing, "CR4 bits went missing: %lx!?\n",
> >  			  bits_missing);
> > +		if (val & X86_CR4_FSGSBASE) {
> > +			WARN_ONCE(1, "CR4 unexpectedly set FSGSBASE!?\n");
> 
> Like this will actually be noticed by anyone who calls this?  What is a
> user supposed to do about this?

In the long term they would need to apply the proper patches
for FSGSBASE.

> 
> What about those systems that panic-on-warn?

I assume they're ok with "panic on root hole"

> 
> > +			val &= ~X86_CR4_FSGSBASE;
> 
> So you just prevented them from setting this, thereby fixing up their
> broken code that will never be fixed because you did this?  Why do this?

If they rely on the functionality they will apply the proper patches
then. Or at least they will be aware that they have a root hole,
which they are currently not.

-Andi

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Kees Cook]

On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> What about those systems that panic-on-warn?

This is (modulo the general discussion about whether it's the right
way to check) the correct use for WARN*(). It's an undesirable system
state; people choosing panic-on-warn want this:
https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on

-- 
Kees Cook

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Kees Cook]

On Tue, May 26, 2020 at 08:48:35AM -0700, Andi Kleen wrote:
> On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > +		if (val & X86_CR4_FSGSBASE) {
> > > +			WARN_ONCE(1, "CR4 unexpectedly set FSGSBASE!?\n");
> > 
> > What about those systems that panic-on-warn?
> 
> I assume they're ok with "panic on root hole"

Exactly. :) The pinning infrastructure is pretty small; will that just
get backported? (Also, we can probably rework the pinning to avoid the
special-casing and use a mask/value pair to notice a bit getting turned
_on_ as well...)

-- 
Kees Cook

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Greg KH]

On Tue, May 26, 2020 at 08:48:35AM -0700, Andi Kleen wrote:
> On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > > 
> > > Since there seem to be kernel modules floating around that set
> > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > CR4 pinning just checks that bits are set, this also checks
> > > that the FSGSBASE bit is not set, and if it is clears it again.
> > 
> > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > modules now?  
> 
> Well it's a specific case where we know they're opening a root hole
> unintentionally. This is just an pragmatic attempt to protect the users in the 
> short term.

Can't you just go and fix those out-of-tree kernel modules instead?
What's keeping you all from just doing that instead of trying to force
the kernel to play traffic cop?

thanks,

greg k-h

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Kees Cook]

On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Since there seem to be kernel modules floating around that set
> FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> CR4 pinning just checks that bits are set, this also checks
> that the FSGSBASE bit is not set, and if it is clears it again.
> 
> Note this patch will need to be undone when the full FSGSBASE
> patches are merged. But it's a reasonable solution for v5.2+
> stable at least. Sadly the older kernels don't have the necessary
> infrastructure for this (although a simpler version of this
> could be added there too)
> 
> Cc: stable@vger.kernel.org # v5.2+
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  arch/x86/kernel/cpu/common.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index bed0cb83fe24..1f5b7871ae9a 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -385,6 +385,11 @@ void native_write_cr4(unsigned long val)
>  		/* Warn after we've set the missing bits. */
>  		WARN_ONCE(bits_missing, "CR4 bits went missing: %lx!?\n",
>  			  bits_missing);
> +		if (val & X86_CR4_FSGSBASE) {
> +			WARN_ONCE(1, "CR4 unexpectedly set FSGSBASE!?\n");
> +			val &= ~X86_CR4_FSGSBASE;
> +			goto set_register;
> +		}
>  	}
>  }
>  EXPORT_SYMBOL(native_write_cr4);
> -- 
> 2.25.4
> 

And if this is going to be more permanent, we can separate the mask
(untested):


diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index bed0cb83fe24..ead64f7420a5 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -347,6 +347,8 @@ static __always_inline void setup_umip(struct cpuinfo_x86 *c)
 	cr4_clear_bits(X86_CR4_UMIP);
 }
 
+static const unsigned long cr4_pinned_mask =
+	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP | X86_CR4_FSGSBASE;
 static DEFINE_STATIC_KEY_FALSE_RO(cr_pinning);
 static unsigned long cr4_pinned_bits __ro_after_init;
 
@@ -371,20 +373,20 @@ EXPORT_SYMBOL(native_write_cr0);
 
 void native_write_cr4(unsigned long val)
 {
-	unsigned long bits_missing = 0;
+	unsigned long bits_changed = 0;
 
 set_register:
 	asm volatile("mov %0,%%cr4": "+r" (val), "+m" (cr4_pinned_bits));
 
 	if (static_branch_likely(&cr_pinning)) {
-		if (unlikely((val & cr4_pinned_bits) != cr4_pinned_bits)) {
-			bits_missing = ~val & cr4_pinned_bits;
-			val |= bits_missing;
+		if (unlikely((val & cr4_pinned_mask) != cr4_pinned_bits)) {
+			bits_changed = ~val & cr4_pinned_mask;
+			val = (val & ~cr4_pinned_mask) | cr4_pinned_bits;
 			goto set_register;
 		}
 		/* Warn after we've set the missing bits. */
-		WARN_ONCE(bits_missing, "CR4 bits went missing: %lx!?\n",
-			  bits_missing);
+		WARN_ONCE(bits_changed, "pinned CR4 bits changed: %lx!?\n",
+			  bits_changed);
 	}
 }
 EXPORT_SYMBOL(native_write_cr4);
@@ -396,7 +398,7 @@ void cr4_init(void)
 	if (boot_cpu_has(X86_FEATURE_PCID))
 		cr4 |= X86_CR4_PCIDE;
 	if (static_branch_likely(&cr_pinning))
-		cr4 |= cr4_pinned_bits;
+		cr4 = (cr4 & ~cr4_pinned_mask) | cr4_pinned_bits;
 
 	__write_cr4(cr4);
 
@@ -411,10 +413,7 @@ void cr4_init(void)
  */
 static void __init setup_cr_pinning(void)
 {
-	unsigned long mask;
-
-	mask = (X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP);
-	cr4_pinned_bits = this_cpu_read(cpu_tlbstate.cr4) & mask;
+	cr4_pinned_bits = this_cpu_read(cpu_tlbstate.cr4) & cr4_pinned_mask;
 	static_key_enable(&cr_pinning.key);
 }
 

-- 
Kees Cook

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Wojtek Porczyk]

[-- Attachment #1: Type: text/plain, Size: 3014 bytes --]

On Tue, May 26, 2020 at 06:32:35PM +0200, Greg KH wrote:
> On Tue, May 26, 2020 at 08:48:35AM -0700, Andi Kleen wrote:
> > On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > > From: Andi Kleen <ak@linux.intel.com>
> > > > 
> > > > Since there seem to be kernel modules floating around that set
> > > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > > CR4 pinning just checks that bits are set, this also checks
> > > > that the FSGSBASE bit is not set, and if it is clears it again.
> > > 
> > > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > > modules now?  
> > 
> > Well it's a specific case where we know they're opening a root hole
> > unintentionally. This is just an pragmatic attempt to protect the users in the 
> > short term.
> 
> Can't you just go and fix those out-of-tree kernel modules instead?
> What's keeping you all from just doing that instead of trying to force
> the kernel to play traffic cop?

We'd very much welcome any help really, but we're under impression that this
couldn't be done correctly in a module, so this hack occured.

This was written in 2015 as part of original (research) codebase for those
reasons:
- A module is easier to deploy by scientists, who are no kernel developers and
  no sysadmins either, so applying patchset and recompiling kernel is a big
  ask.
- It has no implications on security in SGX/Graphene threat model and in
  expected deployment scenario.
- This had no meaning to the actual research being done, so it wasn't cared
  about.

Let me expand the second point, because I understand both the module and the
explanation looks wrong.

Graphene is intended to be run in a cloud, where the CPU time is sold in
a form of virtual machine, so the VM kernel, which would load this module, is
not trusted by hardware owner, so s/he don't care. But the owner of the
enclave also doesn't care, because SGX' threat model assumes adversary who is
capable of arbitrary code execution in both kernel and userspace outside
enclave. So the kernel immediately outside the enclave is a no-man's land,
untrusted by both sides and forsaken, reduced to a compatibility layer
between x86 and ELF.

I acknowledge this is unusual threat model and certainly to mainline
developers, who rarely encounter userspace that is more trusted than kernel.

What we've failed at is to properly explain this, because if someone loads
this module outside of this expected scenario, will certainly be exposed to
a gaping root hole. Therefore we acknowledge this patch and as part of
Graphene we'll probably maintain a patchset, until the support is upstream.
Right now this will take us some time to change from our current kernel
interfaces.


-- 
pozdrawiam / best regards
Wojtek Porczyk
Graphene / Invisible Things Lab
 
 I do not fear computers,
 I fear lack of them.
    -- Isaac Asimov

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Greg KH]

On Tue, May 26, 2020 at 09:15:04AM -0700, Kees Cook wrote:
> On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > What about those systems that panic-on-warn?
> 
> This is (modulo the general discussion about whether it's the right
> way to check) the correct use for WARN*(). It's an undesirable system
> state; people choosing panic-on-warn want this:
> https://www.kernel.org/doc/html/latest/process/deprecated.html#bug-and-bug-on

Ok, tha's good to know, thanks for that.

greg k-h

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Andi Kleen]

> And if this is going to be more permanent, we can separate the mask
> (untested):

The FSGSBASE one should not be permanent, it will be replaced
with the full FSGSBASE patches that set that bit correctly.

I was a bit wary of enforcing it for all bits, there might be other
CR4 bits which have benign uses. But I guess the risk of breaking
something there is low.

-Andi


> 
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index bed0cb83fe24..ead64f7420a5 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -347,6 +347,8 @@ static __always_inline void setup_umip(struct cpuinfo_x86 *c)
>  	cr4_clear_bits(X86_CR4_UMIP);
>  }
>  
> +static const unsigned long cr4_pinned_mask =
> +	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP | X86_CR4_FSGSBASE;
>  static DEFINE_STATIC_KEY_FALSE_RO(cr_pinning);
>  static unsigned long cr4_pinned_bits __ro_after_init;
>  
> @@ -371,20 +373,20 @@ EXPORT_SYMBOL(native_write_cr0);
>  
>  void native_write_cr4(unsigned long val)
>  {
> -	unsigned long bits_missing = 0;
> +	unsigned long bits_changed = 0;
>  
>  set_register:
>  	asm volatile("mov %0,%%cr4": "+r" (val), "+m" (cr4_pinned_bits));
>  
>  	if (static_branch_likely(&cr_pinning)) {
> -		if (unlikely((val & cr4_pinned_bits) != cr4_pinned_bits)) {
> -			bits_missing = ~val & cr4_pinned_bits;
> -			val |= bits_missing;
> +		if (unlikely((val & cr4_pinned_mask) != cr4_pinned_bits)) {
> +			bits_changed = ~val & cr4_pinned_mask;
> +			val = (val & ~cr4_pinned_mask) | cr4_pinned_bits;
>  			goto set_register;
>  		}
>  		/* Warn after we've set the missing bits. */
> -		WARN_ONCE(bits_missing, "CR4 bits went missing: %lx!?\n",
> -			  bits_missing);
> +		WARN_ONCE(bits_changed, "pinned CR4 bits changed: %lx!?\n",
> +			  bits_changed);
>  	}
>  }
>  EXPORT_SYMBOL(native_write_cr4);
> @@ -396,7 +398,7 @@ void cr4_init(void)
>  	if (boot_cpu_has(X86_FEATURE_PCID))
>  		cr4 |= X86_CR4_PCIDE;
>  	if (static_branch_likely(&cr_pinning))
> -		cr4 |= cr4_pinned_bits;
> +		cr4 = (cr4 & ~cr4_pinned_mask) | cr4_pinned_bits;
>  
>  	__write_cr4(cr4);
>  
> @@ -411,10 +413,7 @@ void cr4_init(void)
>   */
>  static void __init setup_cr_pinning(void)
>  {
> -	unsigned long mask;
> -
> -	mask = (X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP);
> -	cr4_pinned_bits = this_cpu_read(cpu_tlbstate.cr4) & mask;
> +	cr4_pinned_bits = this_cpu_read(cpu_tlbstate.cr4) & cr4_pinned_mask;
>  	static_key_enable(&cr_pinning.key);
>  }
>  
> 
> -- 
> Kees Cook
> 

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Greg KH]

On Tue, May 26, 2020 at 07:24:03PM +0200, Wojtek Porczyk wrote:
> On Tue, May 26, 2020 at 06:32:35PM +0200, Greg KH wrote:
> > On Tue, May 26, 2020 at 08:48:35AM -0700, Andi Kleen wrote:
> > > On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > > > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > 
> > > > > Since there seem to be kernel modules floating around that set
> > > > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > > > CR4 pinning just checks that bits are set, this also checks
> > > > > that the FSGSBASE bit is not set, and if it is clears it again.
> > > > 
> > > > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > > > modules now?  
> > > 
> > > Well it's a specific case where we know they're opening a root hole
> > > unintentionally. This is just an pragmatic attempt to protect the users in the 
> > > short term.
> > 
> > Can't you just go and fix those out-of-tree kernel modules instead?
> > What's keeping you all from just doing that instead of trying to force
> > the kernel to play traffic cop?
> 
> We'd very much welcome any help really, but we're under impression that this
> couldn't be done correctly in a module, so this hack occured.

Really?  How is this hack anything other than a "prevent a kernel module
from doing something foolish" change?

Why can't you just change the kernel module's code to not do this?  What
prevents that from happening right now which would prevent the need to
change a core api from being abused in such a way?

> This was written in 2015 as part of original (research) codebase for those
> reasons:
> - A module is easier to deploy by scientists, who are no kernel developers and
>   no sysadmins either, so applying patchset and recompiling kernel is a big
>   ask.
> - It has no implications on security in SGX/Graphene threat model and in
>   expected deployment scenario.
> - This had no meaning to the actual research being done, so it wasn't cared
>   about.
> 
> Let me expand the second point, because I understand both the module and the
> explanation looks wrong.
> 
> Graphene is intended to be run in a cloud, where the CPU time is sold in
> a form of virtual machine, so the VM kernel, which would load this module, is
> not trusted by hardware owner, so s/he don't care. But the owner of the
> enclave also doesn't care, because SGX' threat model assumes adversary who is
> capable of arbitrary code execution in both kernel and userspace outside
> enclave. So the kernel immediately outside the enclave is a no-man's land,
> untrusted by both sides and forsaken, reduced to a compatibility layer
> between x86 and ELF.
> 
> I acknowledge this is unusual threat model and certainly to mainline
> developers, who rarely encounter userspace that is more trusted than kernel.
> 
> What we've failed at is to properly explain this, because if someone loads
> this module outside of this expected scenario, will certainly be exposed to
> a gaping root hole. Therefore we acknowledge this patch and as part of
> Graphene we'll probably maintain a patchset, until the support is upstream.
> Right now this will take us some time to change from our current kernel
> interfaces.

I'm sorry, but I still do not understand.  Your kernel module calls the
core with this bit being set, and this new kernel patch is there to
prevent the bit from being set and will WARN_ON() if it happens.  Why
can't you just change your module code to not set the bit?

Do you have a pointer to the kernel module code that does this operation
which this core kernel change will try to prevent from happening?

thanks,

greg k-h

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Peter Zijlstra]

On Tue, May 26, 2020 at 04:14:25PM -0700, Andi Kleen wrote:
> > And if this is going to be more permanent, we can separate the mask
> > (untested):
> 
> The FSGSBASE one should not be permanent, it will be replaced
> with the full FSGSBASE patches that set that bit correctly.

Well, even with full FSGSBASE patches on, that CR4 bit should never get
changed after boot.

Re: [PATCH v1] x86: Pin cr4 FSGSBASE

[Wojtek Porczyk]

[-- Attachment #1: Type: text/plain, Size: 3701 bytes --]

On Wed, May 27, 2020 at 09:07:55AM +0200, Greg KH wrote:
> On Tue, May 26, 2020 at 07:24:03PM +0200, Wojtek Porczyk wrote:
> > On Tue, May 26, 2020 at 06:32:35PM +0200, Greg KH wrote:
> > > On Tue, May 26, 2020 at 08:48:35AM -0700, Andi Kleen wrote:
> > > > On Tue, May 26, 2020 at 08:56:18AM +0200, Greg KH wrote:
> > > > > On Mon, May 25, 2020 at 10:28:48PM -0700, Andi Kleen wrote:
> > > > > > From: Andi Kleen <ak@linux.intel.com>
> > > > > > 
> > > > > > Since there seem to be kernel modules floating around that set
> > > > > > FSGSBASE incorrectly, prevent this in the CR4 pinning. Currently
> > > > > > CR4 pinning just checks that bits are set, this also checks
> > > > > > that the FSGSBASE bit is not set, and if it is clears it again.
> > > > > 
> > > > > So we are trying to "protect" ourselves from broken out-of-tree kernel
> > > > > modules now?  
> > > > 
> > > > Well it's a specific case where we know they're opening a root hole
> > > > unintentionally. This is just an pragmatic attempt to protect the users in the 
> > > > short term.
> > > 
> > > Can't you just go and fix those out-of-tree kernel modules instead?
> > > What's keeping you all from just doing that instead of trying to force
> > > the kernel to play traffic cop?
> > 
> > We'd very much welcome any help really, but we're under impression that this
> > couldn't be done correctly in a module, so this hack occured.
> 
> Really?  How is this hack anything other than a "prevent a kernel module
> from doing something foolish" change?

By "this hack" I meant our module [1], which just enables FSGSBASE bit without
doing everything else that Sasha's patchset does, and that "everything else"
is there to prevent a gaping root hoole.

Original author wanted module for the reason snipped below, but Sasha's
patchset can't be packaged into module. I'll be happy to be corrected on
this point, I personally have next to no kernel programming experience.

This kernel change I think is correct, because if kernel assumes some
invariants, it's a good idea to actually enforce them, isn't it? So we don't
have anything against this kernel change. We'll have to live with it, with our
hand forced.

> Why can't you just change the kernel module's code to not do this?  What
> prevents that from happening right now which would prevent the need to
> change a core api from being abused in such a way?
[snip]
> I'm sorry, but I still do not understand.  Your kernel module calls the
> core with this bit being set, and this new kernel patch is there to
> prevent the bit from being set and will WARN_ON() if it happens.  Why
> can't you just change your module code to not set the bit?

Because we need userspace access to wrfsbase, which this bit enables:

https://github.com/oscarlab/graphene/blob/58c53ad747579225bf29e3506d883586ff4b8eee/Pal/src/host/Linux-SGX/sgx_api.h#L94-L98
https://github.com/oscarlab/graphene/blob/0dd84ddf943d256e5494f07cb41b1d0ece847c1a/Pal/src/host/Linux-SGX/enclave_entry.S#L675
https://github.com/oscarlab/graphene/blob/e4e16aa10e3c2221170aee7da66370507cc52428/Pal/src/host/Linux-SGX/db_misc.c#L69

> Do you have a pointer to the kernel module code that does this operation
> which this core kernel change will try to prevent from happening?

Sure: https://github.com/oscarlab/graphene-sgx-driver/blob/a73de5fed96fc330fc0417d262ba5e87fea128c2/gsgx.c#L32-L39

The whole module is 86 sloc, and the sole purpose is to set this bit on load
and clear on unload.

[1] ^


-- 
pozdrawiam / best regards
Wojtek Porczyk
Graphene / Invisible Things Lab
 
 I do not fear computers,
 I fear lack of them.
    -- Isaac Asimov

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]