linux-kernel - Re: [PATCH v1] x86: Pin cr4 FSGSBASE

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200526154835.GW499505@tassilo.jf.intel.com>
Date:   Tue, 26 May 2020 08:48:35 -0700
From:   Andi Kleen <ak@...ux.intel.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Andi Kleen <andi@...stfloor.org>, x86@...nel.org,
        keescook@...omium.org, linux-kernel@...r.kernel.org,
        sashal@...nel.org, stable@...r.kernel.org
Subject: Re: [PATCH v1] x86: Pin cr4 FSGSBASE

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@...ux.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