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

[Borislav Petkov]

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@xxxxxxxxx>
> Tested-by: John Allen <john.allen@xxxxxxx>
> Tested-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Acked-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
> Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>

I think your SOB should come last:

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

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.

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