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

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

[Bill Roberts]

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.

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;
 	}
 
 	/* Do not allow to change locked features */
-- 
2.54.0

[PATCH 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Bill Roberts]

One should not be able to use ARCH_SHSTK_UNLOCK via arch_prctl to twiddle
feature bits, test that this is the case.

Signed-off-by: Bill Roberts <bill.roberts@arm.com>
---
 tools/testing/selftests/x86/test_shadow_stack.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
index 21af54d5f4ea..82a5fefa9df5 100644
--- a/tools/testing/selftests/x86/test_shadow_stack.c
+++ b/tools/testing/selftests/x86/test_shadow_stack.c
@@ -979,6 +979,14 @@ int main(int argc, char *argv[])
 {
 	int ret = 0;
 
+	/* test that we can't use unlock to set shadow stack */
+	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
+		printf("[SKIP]\tCould enable Shadow stack via UNLOCK\n");
+		return 1;
+	}
+
+	printf("[OK]\tCouldn't enable Shadow stack via UNLOCK\n");
+
 	if (ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK)) {
 		printf("[SKIP]\tCould not enable Shadow stack\n");
 		return 1;
-- 
2.54.0

Re: [PATCH 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Edgecombe, Rick P]

On Mon, 2026-06-15 at 14:51 -0500, Bill Roberts wrote:
> One should not be able to use ARCH_SHSTK_UNLOCK via arch_prctl to twiddle
> feature bits, test that this is the case.
> 
> Signed-off-by: Bill Roberts <bill.roberts@arm.com>
> ---
>  tools/testing/selftests/x86/test_shadow_stack.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
> index 21af54d5f4ea..82a5fefa9df5 100644
> --- a/tools/testing/selftests/x86/test_shadow_stack.c
> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
> @@ -979,6 +979,14 @@ int main(int argc, char *argv[])
>  {
>  	int ret = 0;
>  
> +	/* test that we can't use unlock to set shadow stack */
> +	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
> +		printf("[SKIP]\tCould enable Shadow stack via UNLOCK\n");
> +		return 1;
> +	}

More generally, we should not be able to use ARCH_SHSTK_UNLOCK here. Can we make
the comment generic and not checking for a specific bug? Because it does more
than that.

> +
> +	printf("[OK]\tCouldn't enable Shadow stack via UNLOCK\n");
> +
>  	if (ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK)) {
>  		printf("[SKIP]\tCould not enable Shadow stack\n");
>  		return 1;

Re: [PATCH 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Bill Roberts]

On 6/29/26 4:46 PM, Edgecombe, Rick P wrote:
> On Mon, 2026-06-15 at 14:51 -0500, Bill Roberts wrote:
>> One should not be able to use ARCH_SHSTK_UNLOCK via arch_prctl to twiddle
>> feature bits, test that this is the case.
>>
>> Signed-off-by: Bill Roberts <bill.roberts@arm.com>
>> ---
>>   tools/testing/selftests/x86/test_shadow_stack.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/tools/testing/selftests/x86/test_shadow_stack.c b/tools/testing/selftests/x86/test_shadow_stack.c
>> index 21af54d5f4ea..82a5fefa9df5 100644
>> --- a/tools/testing/selftests/x86/test_shadow_stack.c
>> +++ b/tools/testing/selftests/x86/test_shadow_stack.c
>> @@ -979,6 +979,14 @@ int main(int argc, char *argv[])
>>   {
>>   	int ret = 0;
>>   
>> +	/* test that we can't use unlock to set shadow stack */
>> +	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
>> +		printf("[SKIP]\tCould enable Shadow stack via UNLOCK\n");
>> +		return 1;
>> +	}
> More generally, we should not be able to use ARCH_SHSTK_UNLOCK here. Can we make
> the comment generic and not checking for a specific bug? Because it does more
> than that.

Hey Rick, not really sure how I could make the git commit message and 
the comment
more generic, as they seem really generic, can you please be more 
specific or give me
a blurb you're looking for here?

>
>> +
>> +	printf("[OK]\tCouldn't enable Shadow stack via UNLOCK\n");
>> +
>>   	if (ARCH_PRCTL(ARCH_SHSTK_ENABLE, ARCH_SHSTK_SHSTK)) {
>>   		printf("[SKIP]\tCould not enable Shadow stack\n");
>>   		return 1;

Re: [PATCH 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Edgecombe, Rick P]

On Mon, 2026-06-29 at 12:09 -0500, Bill Roberts wrote:
> > > +	/* test that we can't use unlock to set shadow stack */
> > > +	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
> > > +		printf("[SKIP]\tCould enable Shadow stack via UNLOCK\n");
> > > +		return 1;
> > > +	}
> > More generally, we should not be able to use ARCH_SHSTK_UNLOCK here. Can we
> > make
> > the comment generic and not checking for a specific bug? Because it does
> > more
> > than that.
> 
> Hey Rick, not really sure how I could make the git commit message and 
> the comment
> more generic, as they seem really generic, can you please be more 
> specific or give me
> a blurb you're looking for here?

I was thinking that the comment was specifically about ARCH_SHSTK_UNLOCK being
able to set shadow stack. But it is really testing whether ARCH_SHSTK_UNLOCK can
be called at all. So maybe:

/*
 * Test that ARCH_SHSTK_UNLOCK can't be called
 * from normal (non-ptrace) context.
 */

Re: [PATCH 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Edgecombe, Rick P]

On Mon, 2026-06-15 at 14:51 -0500, Bill Roberts wrote:
> +	/* test that we can't use unlock to set shadow stack */
> +	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
> +		printf("[SKIP]\tCould enable Shadow stack via UNLOCK\n");

Should it be a FAIL? And if this path is taken the test will crash because
shadow stack got accidentally enabled and we can't return from this function. It
can "goto out" and disable shadow stack before returning.

> +		return 1;
> +	}
> +
> +	printf("[OK]\tCouldn't enable Shadow stack via UNLOCK\n");
> +

Re: [PATCH 2/2] selftest/x86: test ARCH_SHSTK_UNLOCK

[Bill Roberts]

On 6/29/26 4:57 PM, Edgecombe, Rick P wrote:
> On Mon, 2026-06-15 at 14:51 -0500, Bill Roberts wrote:
>> +	/* test that we can't use unlock to set shadow stack */
>> +	if (!ARCH_PRCTL(ARCH_SHSTK_UNLOCK, ARCH_SHSTK_SHSTK)) {
>> +		printf("[SKIP]\tCould enable Shadow stack via UNLOCK\n");
> Should it be a FAIL? And if this path is taken the test will crash because
> shadow stack got accidentally enabled and we can't return from this function. It
> can "goto out" and disable shadow stack before returning.

Yeah Rick, we can do that, not sure why I didn't do that in V1 :-p

>> +		return 1;
>> +	}
>> +
>> +	printf("[OK]\tCouldn't enable Shadow stack via UNLOCK\n");
>> +

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

[Edgecombe, Rick P]

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 */

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

[Bill Roberts]

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 */
>