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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] x86/shstk: arch_set_shadow_stack_status() evaluates current instead of target task t
- [Low] x86/shstk: unreachable rollback logic in arch_set_shadow_stack_status()
--

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

x86/shstk: support via prctl
    
This commit wires up the generic prctl handlers for shadow stack on x86,
mapping PR_SET_SHADOW_STACK_STATUS and related operations to the internal
CET helper routines. This allows userspace to manage shadow stacks via a
cross-architecture interface while maintaining compatibility with arch_prctl.

> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 0ca64900192f8..249c4998732ac 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
[ ... ]
> @@ -630,3 +631,85 @@ 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);

[Severity: Medium]
Does this code incorrectly evaluate the shadow stack status of current instead
of the target task t?

The features_enabled() helper strictly evaluates the features of the current
task via current->thread.features. If arch_set_shadow_stack_status() is ever
called to configure a remote task's shadow stack where t != current, it will
evaluate the caller's state rather than the target's state. This could
potentially short-circuit and silently fail to configure the target task.

> +
> +	/*
> +	 * One must explicitly perform enable/disable calls for the whole feature set so
> +	 * that the locked bits are checked. It is also important to capture entry state
> +	 * for roll-back if needed. However, we must NOT try to enable or disable a feature
> +	 * unless there is a change in feature status from old to new state. This is
> +	 * because shstk_prctl(), correctly, checks locked bits before feature enabled/disabled
> +	 * short circuit.
> +	 */
> +	switch (status) {
[ ... ]
> +	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)) {

[Severity: Medium]
Will this again erroneously evaluate the WRSS feature state of current
instead of the target task t?

> +			rc = shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_WRSS);
> +			if (rc && !shstk_was_enabled)
> +				shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);

[Severity: Low]
Is this rollback for ARCH_SHSTK_SHSTK unreachable?

ARCH_SHSTK_WRSS can only be enabled if ARCH_SHSTK_SHSTK is already enabled.
Because of this, if features_enabled(ARCH_SHSTK_WRSS) evaluates to true
allowing entry into this block, shstk_was_enabled must also be true.
Therefore, !shstk_was_enabled is always false in this context, making this
rollback structurally dead code.

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

[Severity: Medium]
Will this also erroneously evaluate the WRSS feature state of current
instead of the target task t?

> +			rc = shstk_prctl(t, ARCH_SHSTK_ENABLE, ARCH_SHSTK_WRSS);
> +			if (rc && !shstk_was_enabled)
> +				shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> +
> +			return rc;
> +		}
> +
> +		return 0;

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