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

[Bill Roberts <bill.roberts@foss.arm.com>]

On 6/29/26 4:44 PM, Edgecombe, Rick P wrote:
> 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.
I'll make the message more clear on the v2 respin.
>
>> 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.
>
> Ha, I had that initially, but kept the lowest line count change that fixed the issue.
> Ill add that to v2.
>
>>   	}
>>   
>>   	/* Do not allow to change locked features */
>