Re: [PATCH v7 39/41] x86: Add PTRACE interface for shadow stack

Borislav Petkov
2023-03-11T15:06:13Z

On Mon, Feb 27, 2023 at 02:29:55PM -0800, Rick Edgecombe wrote:
> The only downside to not having a generic supervisor xfeature regset,
> is that apps need to be enlightened of any new supervisor xfeature
> exposed this way (i.e. they can't try to have generic save/restore
> logic). But maybe that is a good thing, because they have to think
> through each new xfeature instead of encountering issues when new a new

Remove the first "new".

> supervisor xfeature was added.
>
> By adding a shadow stack regset, it also has the effect of including the
> shadow stack state in a core dump, which could be useful for debugging.
>
> The shadow stack specific xstate includes the SSP, and the shadow stack
> and WRSS enablement status. Enabling shadow stack or wrss in the kernel
^^^^

"WRSS"

> involves more than just flipping the bit. The kernel is made aware that
> it has to do extra things when cloning or handling signals. That logic
> is triggered off of separate feature enablement state kept in the task
> struct. So the flipping on HW shadow stack enforcement without notifying
> the kernel to change its behavior would severely limit what an application
> could do without crashing, and the results would depend on kernel
> internal implementation details. There is also no known use for controlling
> this state via prtace today. So only expose the SSP, which is something

Unknown word [prtace] in commit message.
Suggestions: ['ptrace'

> that userspace already has indirect control over.
>
> Tested-by: Pengfei Xu <pengfei.xu@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>

I think your SOB should come last:

...
Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

Pls check whole set.

> +#ifdef CONFIG_X86_USER_SHADOW_STACK
> +int ssp_active(struct task_struct *target, const struct user_regset *regset)
> +{
> + if (target->thread.features & ARCH_SHSTK_SHSTK)
> + return regset->n;
> +
> + return 0;
> +}
> +
> +int ssp_get(struct task_struct *target, const struct user_regset *regset,
> + struct membuf to)
> +{
> + struct fpu *fpu = &target->thread.fpu;
> + struct cet_user_state *cetregs;
> +
> + if (!boot_cpu_has(X86_FEATURE_USER_SHSTK))

check_for_deprecated_apis: WARNING: arch/x86/kernel/fpu/regset.c:193: Do not use boot_cpu_has() - use cpu_feature_enabled() instead.

Check your whole set pls.

--
Regards/Gruss,
Boris.