Re: [PATCH v7 40/41] x86/shstk: Add ARCH_SHSTK_UNLOCK

[Edgecombe, Rick P]

On Sat, 2023-03-11 at 16:11 +0100, Borislav Petkov wrote:
> On Mon, Feb 27, 2023 at 02:29:56PM -0800, Rick Edgecombe wrote:
> > From: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> > 
> > Userspace loaders may lock features before a CRIU restore operation
> > has
> > the chance to set them to whatever state is required by the process
> > being restored. Allow a way for CRIU to unlock features. Add it as
> > an
> > arch_prctl() like the other shadow stack operations, but restrict
> > it being
> > called by the ptrace arch_pctl() interface.
> > 
> > Tested-by: Pengfei Xu <pengfei.xu@xxxxxxxxx>
> > Tested-by: John Allen <john.allen@xxxxxxx>
> > Tested-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > Acked-by: Mike Rapoport (IBM) <rppt@xxxxxxxxxx>
> 
> That tag is kinda implicit here. Unless he doesn't ACK his own patch.
> :-P

Uhh, right. This was me mindlessly adding his ack to all the patches in
the series.

> 
> > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > Signed-off-by: Mike Rapoport <rppt@xxxxxxxxxxxxx>
> > [Merged into recent API changes, added commit log and docs]
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> 
> ...
> 
> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > index 2faf9b45ac72..3197ff824809 100644
> > --- a/arch/x86/kernel/shstk.c
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -451,9 +451,14 @@ long shstk_prctl(struct task_struct *task, int
> > option, unsigned long features)
> >                return 0;
> >        }
> >   
> > -     /* Don't allow via ptrace */
> > -     if (task != current)
> > +     /* Only allow via ptrace */
> > +     if (task != current) {
> 
> Is that the only case? task != current means ptrace and there's no
> other
> way to do this from userspace?

Not that I could see...

> 
> Isn't there some flag which says that task is ptraced? I think we
> should
> check that one too...

This is how the other arch_prctl()s handle it (if they do handle it,
some don't). So I would think it would be nice to keep all the logic
the same.

I guess the flag might work based on the assumption that if the task is
being ptraced, the arch_prctl() couldn't be coming from anywhere else.
Maybe it should get a nicely named helper that they could all use and
whatever best logic could be commented.

Would this maybe be better as a future cleanup that did the change for
them all?