On Tue, 2022-11-15 at 15:47 +0100, Peter Zijlstra wrote:
> On Fri, Nov 04, 2022 at 03:36:03PM -0700, Rick Edgecombe wrote:
>
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index 71620b77a654..bed7032d35f2 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -450,9 +450,14 @@ long cet_prctl(struct task_struct *task, int
> > option, unsigned long features)
> > return 0;
> > }
> >
> > - /* Don't allow via ptrace */
> > - if (task != current)
> > + /* Only allow via ptrace */
>
> Both the old and new comment are equally useless for they tell us
> nothing the code doesn't already.
>
> Why isn't ptrace allowed to call these, and doesn't that rather leave
> CRIU in a bind, it can unlock but not re-lock the features, leaving
> restored processes more vulnerable than they were.
As I understand it, CRIU does some poking at things via ptrace to setup
a "parasite" which is actual executable code injected in the process.
Then a lot of the restore code actually runs in the process getting
restored.
As for not allowing unlock to be used in the non-ptrace scenario it's
to keep attackers from unlocking the shadow stack disable API and
calling it to disable shadow stack.
As for not allowing the others via ptrace, the call is in the tracing
processes context, so they would operate on their own registers instead
of the tracees.
>
> > + if (task != current) {
> > + if (option == ARCH_CET_UNLOCK &&
> > IS_ENABLED(CONFIG_CHECKPOINT_RESTORE)) {
>
> Why make this conditional on CRIU at all?
Kees asked for it, I think he was worried about attackers using it to
unlock and disable shadow stack. So wanted to lock it down to the
maximum.
>
> > + task->thread.features_locked &= ~features;
> > + return 0;
> > + }
> > return -EINVAL;
> > + }
> >
> > /* Do not allow to change locked features */
> > if (features & task->thread.features_locked)
> > --
> > 2.17.1
> >
