Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.35 as permitted sender) client-ip=23.128.96.35;
From:   Rick Edgecombe <rick.p.edgecombe@intel.com>
To:     x86@kernel.org, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de,
        dave.hansen@linux.intel.com, hpa@zytor.com, luto@kernel.org,
        peterz@infradead.org, linux-kernel@vger.kernel.org
Cc:     rick.p.edgecombe@intel.com, christina.schimpe@intel.com
Subject: [PATCH] x86: Check if shadow stack is active for ssp_get()
Date:   Mon,  4 Dec 2023 11:07:09 -0800
Message-Id: <20231204190709.3907254-1-rick.p.edgecombe@intel.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

The shadow stack regset ->set() handler (ssp_set()) checks the regset
->active() handler (ssp_active()) to verify that shadow stack is active.
When shadow stack is active, the XFEATURE_CET_USER xfeature will not be in
the init state because there is at least one bit set (SHSTK_EN). So
ssp_set() should be able to safely operate on the xfeature in the xsave
buffer after checking ssp_active(). If it finds it is in the init state
anyway, it warns because an unexpected situation has been encountered.

But ssp_get(), the regset_get() handler, doesn't check ssp_active(). This
was under the assumption that all the callers check the ->active()
handler. It is indeed normally the case, but Christina Schimpe reports
that and a warning like the following can be generated:

WARNING: CPU: 5 PID: 1773 at arch/x86/kernel/fpu/regset.c:198 ssp_get+0x89/0xa0
[...]
Call Trace:
<TASK>
? show_regs+0x6e/0x80
? ssp_get+0x89/0xa0
? __warn+0x91/0x150
? ssp_get+0x89/0xa0
? report_bug+0x19d/0x1b0
? handle_bug+0x46/0x80
? exc_invalid_op+0x1d/0x80
? asm_exc_invalid_op+0x1f/0x30
? __pfx_ssp_get+0x10/0x10
? ssp_get+0x89/0xa0
? ssp_get+0x52/0xa0
__regset_get+0xad/0xf0
copy_regset_to_user+0x52/0xc0
ptrace_regset+0x119/0x140
ptrace_request+0x13c/0x850
? wait_task_inactive+0x142/0x1d0
? do_syscall_64+0x6d/0x90
arch_ptrace+0x102/0x300
[...]

It turns out the PTRACE_GETREGSET path does not check ssp_active(). The
issue could be fixed by just removing the warning, but it would be nicer
to rely a check of ssp_active() which is much easier to reason about than
xsave init state logic. So add a ssp_active() check in ssp_get() like
there already is in ssp_set().

Fixes: 2fab02b25ae7 ("x86: Add PTRACE interface for shadow stack")
Reported-by: Christina Schimpe <christina.schimpe@intel.com>
Tested-by: Christina Schimpe <christina.schimpe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
 arch/x86/kernel/fpu/regset.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
index 6bc1eb2a21bd..887b0b8e21e3 100644
--- a/arch/x86/kernel/fpu/regset.c
+++ b/arch/x86/kernel/fpu/regset.c
@@ -190,7 +190,8 @@ int ssp_get(struct task_struct *target, const struct user_regset *regset,
 	struct fpu *fpu = &target->thread.fpu;
 	struct cet_user_state *cetregs;
 
-	if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK))
+	if (!cpu_feature_enabled(X86_FEATURE_USER_SHSTK) ||
+	    !ssp_active(target, regset))
 		return -ENODEV;
 
 	sync_fpstate(fpu);
-- 
2.34.1