Re: [PATCH v2 1/2] x86/shstk: support via prctl

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Operator precedence bug in `!status & PR_SHADOW_STACK_ENABLE` completely breaks the generic shadow stack configuration.
- [High] Non-atomic updates and redundant `shstk_prctl` calls cause spurious `-EPERM` failures when setting status.
- [Medium] `PR_SHADOW_STACK_PUSH` is advertised in the supported mask but completely ignored, causing API state drift.
- [Medium] Missing validation in `arch_lock_shadow_stack_status` allows userspace to lock arbitrary undefined bits.
- [Medium] `arch_get_shadow_stack_status` leaks internal x86-specific `ARCH_SHSTK_*` flags into the generic API.
--

--- Patch [1]: [PATCH v2 1/2] x86/shstk: support via prctl ---
commit 4cf523bcec8bfd8fcae95b19efb75978b5323431
Author: Bill Roberts <[email protected]>

x86/shstk: support via prctl

This commit wires up the generic prctl handlers for shadow stack on x86,
mapping generic PR_SHADOW_STACK_* operations to internal CET routines.

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

[Severity: Medium]
Is PR_SHADOW_STACK_PUSH fully supported here? The flag is included in this
supported mask, but arch_set_shadow_stack_status() below doesn't appear
to map or apply it, meaning attempts to set it will be silently ignored.

> +
> +/* 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;
> +
> +	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 (!status & PR_SHADOW_STACK_ENABLE)

[Severity: High]
Does this condition evaluate correctly? Because of operator precedence,
!status is evaluated before the bitwise AND.

If a user passes 0 to disable all features, (!0) & 1 evaluates to 1,
causing it to erroneously call shstk_prctl() to disable ARCH_SHSTK_SHSTK
and return early, skipping ARCH_SHSTK_WRSS disablement.

> +		return shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_SHSTK);
> +
> +	rc = shstk_prctl(t, ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK);
> +	if (rc)
> +		return rc;
> +
> +	if (status & PR_SHADOW_STACK_WRITE)
> +		return shstk_prctl(t, ARCH_SHSTK_ENABLE, ARCH_SHSTK_WRSS);

[Severity: High]
Will this sequence of shstk_prctl() calls fail if features are already locked?

If userspace has already enabled and locked PR_SHADOW_STACK_ENABLE, and
later calls this function to add PR_SHADOW_STACK_WRITE, the first
shstk_prctl() call for ARCH_SHSTK_SHSTK will return -EPERM because the
feature is locked. This would cause the set operation to abort without
ever enabling the write feature.

> +
> +	return shstk_prctl(t, ARCH_SHSTK_DISABLE, ARCH_SHSTK_WRSS);
> +}
> +
> +/* 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);
> +}

[Severity: Medium]
Should the status mask be validated here before passing it to shstk_prctl()?

Without validation, userspace can pass arbitrary bits into the internal
lock handler. If x86 adds new internal ARCH_SHSTK_ features later, legacy
applications might accidentally lock those future features.

> +
> +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: Medium]
Is it safe to directly leak the internal x86-specific ARCH_SHSTK_* flags
into this generic API?

Currently, ARCH_SHSTK_SHSTK (bit 0) and ARCH_SHSTK_WRSS (bit 1) happen to
align with PR_SHADOW_STACK_ENABLE and PR_SHADOW_STACK_WRITE, but relying
on this 1:1 mapping seems fragile. Any new internal ARCH_SHSTK_* flag
added in the future could be exposed to generic userspace directly.

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