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

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

[Rick Edgecombe]

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

RE: [PATCH] x86: Check if shadow stack is active for ssp_get()

[Schimpe, Christina]

> 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

gdb uses this ptrace call to read the shadow stack register:
https://sourceware.org/pipermail/gdb-patches/2024-December/214322.html

We see the warning regularly when running the testsuite.
Therefore, it would be great if we could merge this patch.

Kind Regards
Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

[tip: x86/urgent] x86/fpu: Check if shadow stack is active for ssp_get()

[tip-bot2 for Rick Edgecombe]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     9b9ab249c4b1babb0d36c9d4f3309cfb6901a15a
Gitweb:        https://git.kernel.org/tip/9b9ab249c4b1babb0d36c9d4f3309cfb6901a15a
Author:        Rick Edgecombe <rick.p.edgecombe@intel.com>
AuthorDate:    Mon, 04 Dec 2023 11:07:09 -08:00
Committer:     Borislav Petkov (AMD) <bp@alien8.de>
CommitterDate: Tue, 07 Jan 2025 12:08:40 +01:00

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

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
on 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>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Tested-by: Christina Schimpe <christina.schimpe@intel.com>
Cc: <stable@kernel.org>
Link: https://lore.kernel.org/r/20231204190709.3907254-1-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 6bc1eb2..887b0b8 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);

Re: [PATCH] x86: Check if shadow stack is active for ssp_get()

[Dave Hansen]

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.

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.

---------------

We also want this to cc:stable, right?

Re: [PATCH] x86: Check if shadow stack is active for ssp_get()

[Edgecombe, Rick P]

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@vger.kernel.org
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>

Re: [PATCH] x86: Check if shadow stack is active for ssp_get()

[Borislav Petkov]

On Tue, Jan 07, 2025 at 07:31:44PM +0000, Edgecombe, Rick P wrote:
> It looks like Boris already queued the original version. Boris, do you want to
> fix up the log or should I send a v2?

Sure, take what I've committed, tweak it and I'll replace it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette