Re: [PATCH v5 4/6] x86/shstk: support via prctl

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] x86/shstk: missing 32-bit check in prctl status helpers
  corrupts user stack
- [Medium] x86/shstk: missing X86_FEATURE_USER_SHSTK check in shadow
  stack prctl helpers
- [Low] x86/shstk: unreachable rollback path in
  arch_set_shadow_stack_status()
--

commit 9a3ff7cedb6830c75ef0c0aa6dd108c66cfde589
Author: Bill Roberts <bill.roberts@arm.com>

x86/shstk: support via prctl

This commit implements the generic PR_SET_SHADOW_STACK_STATUS,
PR_GET_SHADOW_STACK_STATUS, and PR_LOCK_SHADOW_STACK_STATUS prctl()
operations on x86 using the existing shadow stack helpers.

> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index eb690ba90180c..6dfe310634b02 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c

[ ... ]

> @@ -630,3 +631,82 @@ bool shstk_is_enabled(void)
>  {
>  	return features_enabled(ARCH_SHSTK_SHSTK);
>  }
> +
> +/* Handles the generic prctl interface for PR_SET_SHADOW_STACK_STATUS and its feature bits */
> +int arch_set_shadow_stack_status(struct task_struct *t, unsigned long status)
> +{
> +	int rc;
> +	bool shstk_was_enabled = features_enabled(ARCH_SHSTK_SHSTK);
> +
> +	/*
> +	 * Enable/disable the full feature set and preserve the original state for rollback.
> +	 * Only act on changed features, since shstk_prctl() checks locked bits before checking
> +	 * feature status.
> +	 */
> +	switch (status) {
> +	case 0:
> +		/*
> +		 * Do not explicitly disable WRSS. Disabling shadow stack also
> +		 * disables WRSS, and a locked WRSS must not prevent this.
> +		 */
> +		return shstk_was_enabled ?
> +			shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK) : 0;

[Severity: Medium]
Does returning 0 here when shstk_was_enabled is false, and delegating
arch_lock_shadow_stack_status() and arch_get_shadow_stack_status() to
shstk_prctl() without checking cpu_feature_enabled(X86_FEATURE_USER_SHSTK),
cause these prctl() calls to succeed on CPUs without shadow stack support?

shstk_disable(), shstk_setup(), and wrss_control() all check
!cpu_feature_enabled(X86_FEATURE_USER_SHSTK) and return -EOPNOTSUPP, but
ARCH_SHSTK_STATUS and ARCH_SHSTK_LOCK in shstk_prctl() do not, and case 0
here bypasses shstk_disable() when shadow stack is not enabled.  On systems
without X86_FEATURE_USER_SHSTK (or booted with nousershstk),
PR_GET_SHADOW_STACK_STATUS, PR_LOCK_SHADOW_STACK_STATUS, and
PR_SET_SHADOW_STACK_STATUS(0) return 0 instead of an error, misleading
userspace probing for shadow stack support.

> +
> +	case PR_SHADOW_STACK_ENABLE:
> +		if (!shstk_was_enabled) {
> +			rc = shstk_prctl(t, ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> +			if (rc)
> +				return rc;
> +		}
> +
> +		if (features_enabled(ARCH_SHSTK_WRSS)) {
> +			rc = shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_WRSS);
> +			if (rc && !shstk_was_enabled)
> +				shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> +			return rc;
> +		}

[Severity: Low]
Can the rollback check (if (rc && !shstk_was_enabled)) ever be reached?

wrss_control(true) requires ARCH_SHSTK_SHSTK to be enabled, shstk_disable()
clears both ARCH_SHSTK_SHSTK and ARCH_SHSTK_WRSS, and shstk_setup() only
enables ARCH_SHSTK_SHSTK.  Whenever features_enabled(ARCH_SHSTK_WRSS) is
true here, shstk_was_enabled is already true.

[ ... ]

> +/* Handles the generic prctl interface for PR_LOCK_SHADOW_STACK_STATUS and its feature bits */
> +int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status)
> +{
> +	return shstk_prctl(t, ARCH_SHSTK_LOCK, status);
> +}
> +
> +/*
> + * We assume the prctl() feature bits line up with the arch_prctl() specific ones. If not,
> + * the flags returned via arch_get_shadow_stack_status will be mapped wrong.
> + */
> +static_assert(PR_SHADOW_STACK_ENABLE == ARCH_SHSTK_SHSTK);
> +static_assert(PR_SHADOW_STACK_WRITE  == ARCH_SHSTK_WRSS);
> +int arch_get_shadow_stack_status(struct task_struct *t, unsigned long __user *status)
> +{
> +	return shstk_prctl(t, ARCH_SHSTK_STATUS, (unsigned long)status);
> +}

[Severity: High]
Can a 32-bit compat or x32 caller corrupt 4 bytes past its status variable
via PR_GET_SHADOW_STACK_STATUS?

Previously, shstk_prctl() was only reachable from sys_arch_prctl(), which
rejects in_ia32_syscall() upfront, so shstk_prctl() only checks
in_ia32_syscall() inside shstk_setup().  Because sys_prctl() is wired to
32-bit and x32 syscalls, calling prctl(PR_GET_SHADOW_STACK_STATUS, &status,
0, 0, 0) from 32-bit or x32 userspace reaches shstk_prctl(ARCH_SHSTK_STATUS)
and executes an 8-byte put_user() into a 4-byte userspace unsigned long:

arch/x86/kernel/shstk.c:shstk_prctl() {
	if (option == ARCH_SHSTK_STATUS) {
		return put_user(task->thread.features, (unsigned long __user *)arg2);
	}
    ...
}

32-bit IA32 callers can also lock features via
arch_lock_shadow_stack_status() and call arch_set_shadow_stack_status() to
disable shadow stack, toggle WRSS, or get 0 back for status == 0 instead of
-EINVAL.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109182758.252223-1-bill.roberts@arm.com?part=4