On 9/25/2020 9:31 AM, Andy Lutomirski wrote:

On Fri, Sep 25, 2020 at 7:58 AM Yu-cheng Yu [off-list ref] wrote:

quoted

[...]

quoted

@@ -286,6 +289,37 @@ bool emulate_vsyscall(unsigned long error_code,
         /* Emulate a ret instruction. */
         regs->ip = caller;
         regs->sp += 8;
+
+#ifdef CONFIG_X86_CET
+       if (tsk->thread.cet.shstk_size || tsk->thread.cet.ibt_enabled) {
+               struct cet_user_state *cet;
+               struct fpu *fpu;
+
+               fpu = &tsk->thread.fpu;
+               fpregs_lock();
+
+               if (!test_thread_flag(TIF_NEED_FPU_LOAD)) {
+                       copy_fpregs_to_fpstate(fpu);
+                       set_thread_flag(TIF_NEED_FPU_LOAD);
+               }
+
+               cet = get_xsave_addr(&fpu->state.xsave, XFEATURE_CET_USER);
+               if (!cet) {
+                       fpregs_unlock();
+                       goto sigsegv;

I *think* your patchset tries to keep cet.shstk_size and
cet.ibt_enabled in sync with the MSR, in which case it should be
impossible to get here, but a comment and a warning would be much
better than a random sigsegv.

Yes, it should be impossible to get here.  I will add a comment and a 
warning, but still do sigsegv.  Should this happen, and the function 
return, the app gets a control-protection fault.  Why not let it fail early?

Shouldn't we have a get_xsave_addr_or_allocate() that will never
return NULL but instead will mark the state as in use and set up the
init state if the feature was previously not in use?

We already have a static __raw_xsave_addr(), which returns a pointer to 
the requested xstate.  Maybe we can export __raw_xsave_addr(), if that 
is needed.