Re: [PATCH 03/19] x86/cpufeatures: Enable CET CR4 bit for shadow stack

From: "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
To: "peterz@infradead.org" <peterz@infradead.org>,
	"ebiederm@xmission.com" <ebiederm@xmission.com>,
	"kexec@lists.infradead.org" <kexec@lists.infradead.org>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"keescook@chromium.org" <keescook@chromium.org>,
	"Yu, Yu-cheng" <yu-cheng.yu@intel.com>,
	"x86@kernel.org" <x86@kernel.org>, "Christopherson,,
	Sean" <seanjc@google.com>,
	"Yang, Weijiang" <weijiang.yang@intel.com>,
	"pbonzini@redhat.com" <pbonzini@redhat.com>
Subject: Re: [PATCH 03/19] x86/cpufeatures: Enable CET CR4 bit for shadow stack
Date: Fri, 17 Jun 2022 21:18:47 +0000	[thread overview]
Message-ID: <b1d4d26144e933382a92425164f78367ff92aea5.camel@intel.com> (raw)
In-Reply-To: <YqxnwRn+/c+i1vL6@worktop.programming.kicks-ass.net>

+kexec people

On Fri, 2022-06-17 at 13:38 +0200, Peter Zijlstra wrote:
> On Thu, Jun 16, 2022 at 05:12:47PM +0000, Edgecombe, Rick P wrote:
> > On Thu, 2022-06-16 at 12:24 +0200, Peter Zijlstra wrote:
> > > On Thu, Jun 16, 2022 at 04:46:27AM -0400, Yang Weijiang wrote:
> > > > --- a/arch/x86/include/asm/cpu.h
> > > > +++ b/arch/x86/include/asm/cpu.h
> > > > @@ -74,7 +74,7 @@ void init_ia32_feat_ctl(struct cpuinfo_x86
> > > > *c);
> > > >    static inline void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> > > > {}
> > > >    #endif
> > > >    
> > > > -extern __noendbr void cet_disable(void);
> > > > +extern __noendbr void ibt_disable(void);
> > > >    
> > > >    struct ucode_cpu_info;
> > > >    
> > > > diff --git a/arch/x86/kernel/cpu/common.c
> > > > b/arch/x86/kernel/cpu/common.c
> > > > index c296cb1c0113..86102a8d451e 100644
> > > > --- a/arch/x86/kernel/cpu/common.c
> > > > +++ b/arch/x86/kernel/cpu/common.c
> > > > @@ -598,23 +598,23 @@ __noendbr void ibt_restore(u64 save)
> > > >    
> > > > -__noendbr void cet_disable(void)
> > > > +__noendbr void ibt_disable(void)
> > > >    {
> > > >         if (cpu_feature_enabled(X86_FEATURE_IBT))
> > > >                 wrmsrl(MSR_IA32_S_CET, 0);
> > > 
> > > Not sure about this rename; it really disables all of (S) CET.
> > > 
> > > Specifically, once we do S-SHSTK (after FRED) we might also very
> > > much
> > > need to kill that for kexec.
> > 
> > Sure, what about something like sup_cet_disable()?
> 
> Why bother? Arguably kexec should clear U_CET too.

Hmm, I think you're right. It doesn't look the fpu stuff actually would
reset unknown xfeatures to init. So kernels with Kernel IBT would set
CR4.CET and then MSR_IA32_U_CET might make it to the point where
userspace would run with CET enabled.

It seems like a general kexec problem for when the kernel enables new
xfeatures. I suppose having vector instruction type data stick around
is not going to show up the same way as having new enforcement rules
applied.

But also, looking at this, the existing clearing of MSR_IA32_S_CET is
not sufficient, since it only does it on the cpu doing the kexec. I
think something like the below might be needed. Since per the other
discussion we are going to need to start setting CR4.CET whenever the
HW supports it, for KVM's benefit. So those other CPUs might get
supervisor IBT enabled if we don't clear the msr from every CPU.

diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
index 9730c88530fc..eb57d7f4fa6a 100644
--- a/arch/x86/kernel/crash.c
+++ b/arch/x86/kernel/crash.c
@@ -96,6 +96,12 @@ static void kdump_nmi_callback(int cpu, struct
pt_regs *regs)
        cpu_emergency_stop_pt();
 
        disable_local_APIC();
+
+       /*
+        * Make sure to disable CET features before kexec so the new
kernel
+        * doesn't get surprised by the enforcement.
+        */
+       cet_disable();
 }
 
 void kdump_nmi_shootdown_cpus(void)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 738226472468..de65bac0ae02 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -787,6 +787,12 @@ void __noreturn stop_this_cpu(void *dummy)
         */
        if (cpuid_eax(0x8000001f) & BIT(0))
                native_wbinvd();
+
+       /*
+        * Make sure to disable CET features before kexec so the new
kernel
+        * doesn't get surprised by the enforcement.
+        */
+       cet_disable();
        for (;;) {
                /*
                 * Use native_halt() so that memory contents don't
change
