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