Message-ID: <cf03ec34bd6dfbd8f97a21d3df8c7ccb1b3d78a2.camel@intel.com>
Date: Tue, 7 Jan 2025 19:31:44 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Hansen, Dave" <dave.hansen@...el.com>, "luto@...nel.org"
	<luto@...nel.org>, "dave.hansen@...ux.intel.com"
	<dave.hansen@...ux.intel.com>, "bp@...en8.de" <bp@...en8.de>,
	"peterz@...radead.org" <peterz@...radead.org>, "hpa@...or.com"
	<hpa@...or.com>, "mingo@...hat.com" <mingo@...hat.com>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "x86@...nel.org" <x86@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
CC: "Schimpe, Christina" <christina.schimpe@...el.com>
Subject: Re: [PATCH] x86: Check if shadow stack is active for ssp_get()

On Tue, 2025-01-07 at 10:01 -0800, Dave Hansen wrote:
> Some missing background: The x86 shadow stack support has its own set of
> registers. Those registers are XSAVE-managed, but they are "supervisor
> state components" which means that userspace can't touch them with
> XSAVE/XRSTOR.  It also means that they are not accessible from the
> existing ptrace ABI like the FPU register or GPRs. Thus, there is a new
> ptrace get/set interface for it.
> 
> The ptrace code is also provided an ->active() handler in addition to
> the get/set ones. But this ->active() handler is _not_ checked before
> the get/set handlers are called. This was not understood when shadow
> stack support was put in place.

Ahh, sorry for the confusion. The reason why I specifically mentioned
PTRACE_GETREGSET was because there are two callers of the regset_get() handlers,
one that starts in fill_thread_core_info() where the active handler is checked,
and one for ptrace (PTRACE_GETREGSET), where it is not.

I think the ->active() discussion opens up a little room for confusion, because
the problem we are worried about is xstate specific, not related to all regsets
needing to check ->active(). (i.e. a potential solution is not to add ->active()
checks in the ptrace code generically) We just need to have the comment "This
shouldn't ever be NULL because shadow stack was verified to be enabled above"
actually be true. So I think the discussion of checking ->active() confuses the
actual problem. We only use ssp_active() because it's a handy way of checking
what we need.

> 
> I think I'd also phrase the problem like this:
> 
> As a result, both the set/get handlers can be called with
> XFEATURE_CET_USER in its init state, which would cause get_xsave_addr()
> to return NULL and trigger a WARN_ON(). The ssp_set() handler luckily
> has an ssp_active() check to avoid surprising the kernel with shadow
> stack behavior when the kernel is not read for it (ARCH_SHSTK_SHSTK==0).
> That check just happened to avoid the warning.
> 
> But the ->get() side wasn't so lucky. It can be called with shadow
> stacks disabled, triggering the warning in practice, as reported by
> Christina Schimpe:
> 
> ...
> 
> Ensure that shadow stacks are active in a thread before looking them up
> in the XSAVE buffer. Since ARCH_SHSTK_SHSTK and user_ssp[SHSTK_EN] are
> set at the same time, the active check ensures that there will be
> something to find in the XSAVE buffer.

Yes, it does seem to be an improvement.

> 
> ---------------
> 
> We also want this to cc:stable, right?

Oops, yes.

It looks like Boris already queued the original version. Boris, do you want to
fix up the log or should I send a v2?

All together, with some ->active() tweaks:

x86: Check if shadow stack is active for ssp_get()

The x86 shadow stack support has its own set of registers. Those registers are
XSAVE-managed, but they are "supervisor state components" which means that
userspace can't touch them with XSAVE/XRSTOR.  It also means that they are not
accessible from the existing ptrace ABI like the FPU register or GPRs. Thus,
there is a new ptrace get/set interface for it.

The regset code that ptrace uses provides an ->active() handler in addition to
the get/set ones. For shadow stack this ->active() handler verifies that shadow
stack is enabled via the ARCH_SHSTK_SHSTK bit in the thread struct. The 
->active() handler is checked from some callsites of the regset get/set
handlers, but not the ptrace ones. This was not understood when shadow stack
support was put in place.

As a result, both the set/get handlers can be called with XFEATURE_CET_USER in
its init state, which would cause get_xsave_addr() to return NULL and trigger a
WARN_ON(). The ssp_set() handler luckily has an ssp_active() check to avoid
surprising the kernel with shadow stack behavior when the kernel is not read for
it (ARCH_SHSTK_SHSTK==0). That check just happened to avoid the warning.

But the ->get() side wasn't so lucky. It can be called with shadow stacks
disabled, triggering the warning in practice, as reported by Christina Schimpe:

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
[...]

Ensure that shadow stacks are active in a thread before looking them up
in the XSAVE buffer. Since ARCH_SHSTK_SHSTK and user_ssp[SHSTK_EN] are
set at the same time, the active check ensures that there will be
something to find in the XSAVE buffer.

Cc: stable@...r.kernel.org
Fixes: 2fab02b25ae7 ("x86: Add PTRACE interface for shadow stack")
Reported-by: Christina Schimpe <christina.schimpe@...el.com>
Tested-by: Christina Schimpe <christina.schimpe@...el.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@...el.com>

Powered by blists - more mailing lists