Re: [PATCH v3 4/5] x86/shstk: support via prctl

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Flawed rollback logic destroys an already-enabled shadow stack if enabling WRSS fails.
- [High] Unconditional toggling of WRSS causes spurious failures and partial state application when features are locked.
- [Medium] Setting PR_SHADOW_STACK_WRITE without PR_SHADOW_STACK_ENABLE silently disables the shadow stack.
--

--- Patch [4]: [PATCH v3 4/5] x86/shstk: support via prctl ---
commit b3742dadd5cde35b115774fa09769c5a44a81e76
Author: Bill Roberts <[email protected]>
Subject: x86/shstk: support via prctl

This commit wires up the generic prctl handlers for shadow stack on x86,
mapping PR_SHADOW_STACK_ENABLE, PR_SHADOW_STACK_DISABLE, and
PR_SHADOW_STACK_LOCK to the internal CET helper routines. This provides a
consistent cross-architecture interface for user-space applications to
manage control-flow integrity.

> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 0ca64900192f8..2c2a96b2d9367 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c

[ ... ]

> @@ -630,3 +631,58 @@ bool shstk_is_enabled(void)
>  {
>  	return features_enabled(ARCH_SHSTK_SHSTK);
>  }
> +
> +#define PR_SHADOW_STACK_SUPPORTED_STATUS_MASK \
> +		(PR_SHADOW_STACK_ENABLE | PR_SHADOW_STACK_WRITE)
> +
> +/* 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;
> +	int tmp_rc;
> +
> +	if (status & ~PR_SHADOW_STACK_SUPPORTED_STATUS_MASK)
> +		return -EINVAL;
> +
> +	/*
> +	 * x86 arch_prctl is single bit at a time, so handle these one at time
> +	 * If anything fails, rollback state.
> +	 */
> +	if (!(status & PR_SHADOW_STACK_ENABLE))
> +		return shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);

[Severity: Medium]
Does this silently ignore a request for PR_SHADOW_STACK_WRITE without
PR_SHADOW_STACK_ENABLE?

If an application calls prctl(PR_SET_SHADOW_STACK_STATUS,
PR_SHADOW_STACK_WRITE), the condition (!(status & PR_SHADOW_STACK_ENABLE))
evaluates to true.

It appears this will call shstk_prctl() to disable the shadow stack and
return 0 for success, instead of returning an error like -EINVAL for the
unsupported configuration.

> +
> +	rc = shstk_prctl(t, ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> +	if (rc)
> +		return rc;
> +
> +	if (status & PR_SHADOW_STACK_WRITE) {
> +		rc = shstk_prctl(t, ARCH_SHSTK_ENABLE, ARCH_SHSTK_WRSS);
> +		/* rollback, best attempt, if we actually enabled shadow stack in feature bits */
> +		if (rc && (status & PR_SHADOW_STACK_ENABLE)) {
> +			tmp_rc = shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> +			WARN(tmp_rc, "Could not rollback shadow stack enabled: %d\n", tmp_rc);
> +		}

[Severity: High]
Can this rollback logic unintentionally destroy an already-enabled shadow
stack?

If an application already has its shadow stack enabled, but WRSS is locked
to disabled, a call to prctl(PR_SET_SHADOW_STACK_STATUS,
PR_SHADOW_STACK_ENABLE | PR_SHADOW_STACK_WRITE) will process the SHSTK
enable as a no-op (returning 0).

When it attempts to enable WRSS, shstk_prctl() will return -EPERM due to
the lock. The rollback condition checks (status & PR_SHADOW_STACK_ENABLE)
from the user request rather than checking if SHSTK was actually toggled
during this specific call. Could this cause the pre-existing shadow stack
to be disabled?

Additionally, a concurrent thread locking the shadow stack during the
rollback window could trigger a user-controllable kernel WARN, potentially
leading to a denial of service if panic_on_warn is set.

> +
> +		return rc;
> +
> +	}
> +
> +	return shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_WRSS);
> +}

[Severity: High]
Will this unconditional disablement return spurious errors when features
are locked?

If an application locks WRSS to disabled, and later calls
prctl(PR_SET_SHADOW_STACK_STATUS, PR_SHADOW_STACK_ENABLE) without the
WRITE flag, the shadow stack is successfully enabled.

However, the code falls through to unconditionally call shstk_prctl(t,
ARCH_SHSTK_DISABLE, ARCH_SHSTK_WRSS). Because WRSS is locked,
shstk_prctl() will fail with -EPERM, even though the target state matches
the current disabled state.

Could this return an error to user space after partially applying the
state, potentially leaking the allocation if the application aborts based
on the error?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4