Re: [PATCH v2 1/2] arch/x86: handle ARCH_SHSTK_ENABLE explicitly

[Edgecombe, Rick P]

On Tue, 2026-07-07 at 14:11 -0500, Bill Roberts wrote:
> The arch_prctl() handling for shadow stack operations checks that the
> operation being specified is a specific value, except for the
> ARCH_SHSTK_ENABLE condition, which is treated like a default case in a
> switch.

Seems a bit unspecific "checks that the operation being specified is a specific
value".

> 
> However, the special handling for ARCH_SHSTK_UNLOCK is only performed when
> task != current. As a result, an ARCH_SHSTK_UNLOCK request for the current
> task bypasses the unlock check and falls through to the ARCH_SHSTK_ENABLE
> path, causing the request to be interpreted as an enable operation
> instead.
> 
> To fix this, handle ARCH_SHSTK_ENABLE explicitly rather than relying on it
> as the default case. This fixes the incorrect dispatch and makes the
> operation handling more robust by requiring each operation to be matched
> explicitly.

I don't know if maintainers will be happy with the log. I'd have written it like
this. But no warranty:

   On x86 user shadow stack is controlled via an arch_prctl() interface, in
   order to allow ptrace to actuate some of the options, and allow normal
   syscall execution of arch_prctl() to execute others. Only valid shadow stack
   options, for both ptrace and normal userspace callers, are directed into the
   shstk_prctl() handler via checking in do_arch_prctl_64(). Then in
   shstk_prctl(), it differentiates ptrace callers by checking if task !=
   current.
   
   The trouble starts when the handling of the task == current (normal syscall)
   case treats the enabling option, ARCH_SHSTK_ENABLE, like a default case. It
   reasons that shstk_prctl() has handled all other possible shadow stack
   options, so ARCH_SHSTK_ENABLE can be the only remaining. Or so it thinks.
   Because the ARCH_SHSTK_UNLOCK option is only handled in the ptrace case.
   Since it has no handler in the syscall case, ARCH_SHSTK_UNLOCK can get
   misinterpreted as ARCH_SHSTK_ENABLE by the erroneous default case logic.
   
   The impact is that doing an arch_prtcl(ARCH_SHSTK_UNLOCK) syscall actually
   behaves like an arch_prtcl(ARCH_SHSTK_ENABLE) syscall. This is a surprising
   behavior, but does not expose any control to userspace that it doesn't
   already have.
   
   So drop the buggy default assumptions by checking for ARCH_SHSTK_ENABLE
   explicitly.


> 
> Signed-off-by: Bill Roberts <bill.roberts@xxxxxxx>
> ---

Fix looks good. I think it's correct that this isn't marked for stable.

>  arch/x86/kernel/shstk.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 0ca64900192f..65c896d02379 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -607,11 +607,13 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
>  		return -EINVAL;
>  	}
>  
> -	/* Handle ARCH_SHSTK_ENABLE */
> -	if (features & ARCH_SHSTK_SHSTK)
> -		return shstk_setup();
> -	if (features & ARCH_SHSTK_WRSS)
> -		return wrss_control(true);
> +	if (option == ARCH_SHSTK_ENABLE) {
> +		if (features & ARCH_SHSTK_SHSTK)
> +			return shstk_setup();
> +		if (features & ARCH_SHSTK_WRSS)
> +			return wrss_control(true);
> +	}
> +
>  	return -EINVAL;
>  }
>