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?
