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:

quoted

From: Mike Rapoport <redacted>

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 <redacted>
Tested-by: John Allen <john.allen@amd.com>
Tested-by: Kees Cook <redacted>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>

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.

quoted

Reviewed-by: Kees Cook <redacted>
Signed-off-by: Mike Rapoport <redacted>
[Merged into recent API changes, added commit log and docs]
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

...

quoted

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?