| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <8818b456954644ce609e07d77a65714788ef9098.camel@intel.com>
Date: Thu, 14 Aug 2025 17:03:36 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "mingo@...nel.org" <mingo@...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>, "axboe@...nel.dk" <axboe@...nel.dk>, "tglx@...utronix.de"
<tglx@...utronix.de>, "Mehta, Sohil" <sohil.mehta@...el.com>,
"oleg@...hat.com" <oleg@...hat.com>
CC: "broonie@...nel.org" <broonie@...nel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "x86@...nel.org" <x86@...nel.org>,
"debug@...osinc.com" <debug@...osinc.com>
Subject: Re: [PATCH 5/6] x86/shstk: don't create the shadow stack for
PF_USER_WORKERs
+Mark and Deepak
On Thu, 2025-08-14 at 12:14 +0200, Oleg Nesterov wrote:
> If a features_enabled(ARCH_SHSTK_SHSTK) userspace thread creates a
> PF_USER_WORKER thread, shstk_alloc_thread_stack() allocates the shadow
> stack for no reason, the new (kernel) thread will never return to usermode.
>
> Plus the current code doesn't even look correct, in this case fpu_clone()
> won't call update_fpu_shstk().
The actual SSP register gets set when returning to userspace, it will get this
from MSR_IA32_PL3_SSP, which is restored from the xsave buffer. What I am seeing
is that fpu_clone() clears out the buffer in the 'minimal' case. So the xstate
copy of the SSP should already be zero and the problem is just that a shadow
stack gets allocated when one doesn't need to be.
I agree we don't need to allocate a shadow stack in this case, but I'm not sure
it is right to fully disable shadow stack in thread.features. First of all,
disabling it from shstk_alloc_thread_stack() seems weird. It just handles
allocating shadow stacks.
But also it seems the requirements are very similar to the vfork case where we
don't allocate a shadow stack. If we implement it similarly we can have less
special cases.
Lastly, it doesn't seem there is any way to clone from IO uring today, but there
were proposals. The general idea from the security POV is that copied threads
will keep shadow stack enabled. So it seems to fit better with the concepts
involved to not clear the thread.features.
How about just adding the 'minimal' condition to:
if (clone_flags & CLONE_VFORK) {
shstk->base = 0;
shstk->size = 0;
return 0;
}
...then update all the comments where vfork is called out as the only case that
does this?
>
> Add the new "bool minimal = !!args->fn" argument (which matches that of
> fpu_clone()) to shstk_alloc_thread_stack() and change it to do
> reset_thread_features(tsk) if "minimal" is true.
>
> With this patch ssp_get() -> ssp_active(target) should never return true
> if target->flags & PF_USER_WORKER.
>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>
> ---
> arch/x86/include/asm/shstk.h | 4 ++--
> arch/x86/kernel/process.c | 2 +-
> arch/x86/kernel/shstk.c | 11 ++++++++++-
> 3 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/shstk.h b/arch/x86/include/asm/shstk.h
> index 92d449cc352a..dfb2aeebc25f 100644
> --- a/arch/x86/include/asm/shstk.h
> +++ b/arch/x86/include/asm/shstk.h
> @@ -17,7 +17,7 @@ struct thread_shstk {
> long shstk_prctl(struct task_struct *task, int option, unsigned long arg2);
> void reset_thread_features(struct task_struct *task);
> unsigned long shstk_alloc_thread_stack(struct task_struct *p, unsigned long clone_flags,
> - unsigned long stack_size);
> + bool minimal, unsigned long stack_size);
> void shstk_free(struct task_struct *p);
> int setup_signal_shadow_stack(struct ksignal *ksig);
> int restore_signal_shadow_stack(void);
> @@ -28,7 +28,7 @@ static inline long shstk_prctl(struct task_struct *task, int option,
> unsigned long arg2) { return -EINVAL; }
> static inline void reset_thread_features(struct task_struct *task) {}
> static inline unsigned long shstk_alloc_thread_stack(struct task_struct *p,
> - unsigned long clone_flags,
> + unsigned long clone_flags, bool minimal,
> unsigned long stack_size) { return 0; }
> static inline void shstk_free(struct task_struct *p) {}
> static inline int setup_signal_shadow_stack(struct ksignal *ksig) { return 0; }
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 1b7960cf6eb0..e932e0e53972 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -209,7 +209,7 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> * is disabled, new_ssp will remain 0, and fpu_clone() will know not to
> * update it.
> */
> - new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->stack_size);
> + new_ssp = shstk_alloc_thread_stack(p, clone_flags, args->fn, args->stack_size);
> if (IS_ERR_VALUE(new_ssp))
> return PTR_ERR((void *)new_ssp);
>
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index e6d3b1371b11..3da22c6f5874 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -192,11 +192,20 @@ void reset_thread_features(struct task_struct *tsk)
> }
>
> unsigned long shstk_alloc_thread_stack(struct task_struct *tsk, unsigned long clone_flags,
> - unsigned long stack_size)
> + bool minimal, unsigned long stack_size)
> {
> struct thread_shstk *shstk = &tsk->thread.shstk;
> unsigned long addr, size;
>
> + /*
> + * Kernel threads cloned from userspace thread never return to
> + * usermode.
> + */
> + if (minimal) {
> + reset_thread_features(tsk);
> + return 0;
> + }
> +
> /*
> * If shadow stack is not enabled on the new thread, skip any
> * switch to a new shadow stack.
Powered by blists - more mailing lists