Re: [PATCH 1/2] arch/x86: do not allow unlock to set bits

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

On Mon, 2026-06-15 at 14:51 -0500, Bill Roberts wrote:
> Currently, the code for handling arch_prctl for shadow stack operations
> is written to exit early on all operations but enable. However, the
> check for ARCH_SHSTK_UNLOCK is gated on a check for task != current,
> which means that if current == task, ARCH_SHSTK_UNLOCK can be used to
> set feature bits by virtue of skipping that check.
> 
> This seems not as intended, and the check should first check that the
> operation is ARCH_SHSTK_UNLOCK and then check the task status to
> determine the error code.

I found this log kind of confusing. The problem being fixed is not that
ARCH_SHSTK_UNLOCK can be used when current == task, but rather that it can be
misinterpreted as ARCH_SHSTK_ENABLE. Which is allowed when current == task. So
this is a very strange ABI, but nothing is exposed.

> 
> Signed-off-by: Bill Roberts <bill.roberts@arm.com>
> ---
>  arch/x86/kernel/shstk.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> index 0ca64900192f..664167f94acd 100644
> --- a/arch/x86/kernel/shstk.c
> +++ b/arch/x86/kernel/shstk.c
> @@ -583,12 +583,13 @@ long shstk_prctl(struct task_struct *task, int option, unsigned long arg2)
>  	}
>  
>  	/* Only allow via ptrace */
> -	if (task != current) {
> -		if (option == ARCH_SHSTK_UNLOCK && IS_ENABLED(CONFIG_CHECKPOINT_RESTORE)) {
> -			task->thread.features_locked &= ~features;
> -			return 0;
> -		}
> -		return -EINVAL;
> +	if (option == ARCH_SHSTK_UNLOCK) {
> +		if (task == current)
> +			return -EPERM;
> +		if (!IS_ENABLED(CONFIG_CHECKPOINT_RESTORE))
> +			return -EINVAL;
> +		task->thread.features_locked &= ~features;
> +		return 0;

I think the root cause of this is the slightly odd default treatment of
ARCH_SHSTK_ENABLE. We should refactor it to be a normal case statement that
explicitly has behavior for each ARCH_SHSTK_FOO. This moves in that direction,
but leaves the code brittle.

>  	}
>  
>  	/* Do not allow to change locked features */