diff mbox series

[v7,40/41] x86/shstk: Add ARCH_SHSTK_UNLOCK

Message ID 20230227222957.24501-41-rick.p.edgecombe@intel.com
State New
Headers
Series Shadow stacks for userspace |

Commit Message

Edgecombe, Rick P Feb. 27, 2023, 10:29 p.m. UTC 
  From: Mike Rapoport <rppt@linux.ibm.com>

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@intel.com>
Tested-by: John Allen <john.allen@amd.com>
Tested-by: Kees Cook <keescook@chromium.org>
Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
[Merged into recent API changes, added commit log and docs]
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

---
v4:
 - Add to docs that it is ptrace only.
 - Remove "CET" references

v3:
 - Depend on CONFIG_CHECKPOINT_RESTORE (Kees)
---
 Documentation/x86/shstk.rst       | 4 ++++
 arch/x86/include/uapi/asm/prctl.h | 1 +
 arch/x86/kernel/process_64.c      | 1 +
 arch/x86/kernel/shstk.c           | 9 +++++++--
 4 files changed, 13 insertions(+), 2 deletions(-)

Comments

Borislav Petkov March 11, 2023, 3:11 p.m. UTC | #1 
On Mon, Feb 27, 2023 at 02:29:56PM -0800, Rick Edgecombe wrote:
> From: Mike Rapoport <rppt@linux.ibm.com>
> 
> 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@intel.com>
> Tested-by: John Allen <john.allen@amd.com>
> Tested-by: Kees Cook <keescook@chromium.org>
> Acked-by: Mike Rapoport (IBM) <rppt@kernel.org>

That tag is kinda implicit here. Unless he doesn't ACK his own patch.
:-P

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

...

> 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?

Isn't there some flag which says that task is ptraced? I think we should
check that one too...
Edgecombe, Rick P March 13, 2023, 3:04 a.m. UTC | #2 
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@linux.ibm.com>
> > 
> > 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@intel.com>
> > Tested-by: John Allen <john.allen@amd.com>
> > Tested-by: Kees Cook <keescook@chromium.org>
> > 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.

> 
> > Reviewed-by: Kees Cook <keescook@chromium.org>
> > Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> > [Merged into recent API changes, added commit log and docs]
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> ...
> 
> > 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?
Borislav Petkov March 13, 2023, 11:05 a.m. UTC | #3 
On Mon, Mar 13, 2023 at 03:04:10AM +0000, Edgecombe, Rick P wrote:
> 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?

Yeah, I'm just being overly paranoid.

Because if there's another way to unlock that feature, then this whole
"overhead" we're doing is for nothing.
diff mbox series

Patch

diff --git a/Documentation/x86/shstk.rst b/Documentation/x86/shstk.rst
index f2e6f323cf68..e8ed5fc0f7ae 100644
--- a/Documentation/x86/shstk.rst
+++ b/Documentation/x86/shstk.rst
@@ -73,6 +73,10 @@  arch_prctl(ARCH_SHSTK_LOCK, unsigned long features)
     are ignored. The mask is ORed with the existing value. So any feature bits
     set here cannot be enabled or disabled afterwards.
 
+arch_prctl(ARCH_SHSTK_UNLOCK, unsigned long features)
+    Unlock features. 'features' is a mask of all features to unlock. All
+    bits set are processed, unset bits are ignored. Only works via ptrace.
+
 The return values are as follows. On success, return 0. On error, errno can
 be::
 
diff --git a/arch/x86/include/uapi/asm/prctl.h b/arch/x86/include/uapi/asm/prctl.h
index e31495668056..200efbbe5809 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
@@ -25,6 +25,7 @@ 
 #define ARCH_SHSTK_ENABLE		0x5001
 #define ARCH_SHSTK_DISABLE		0x5002
 #define ARCH_SHSTK_LOCK			0x5003
+#define ARCH_SHSTK_UNLOCK		0x5004
 
 /* ARCH_SHSTK_ features bits */
 #define ARCH_SHSTK_SHSTK		(1ULL <<  0)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 71094c8a305f..d368854fa9c4 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -835,6 +835,7 @@  long do_arch_prctl_64(struct task_struct *task, int option, unsigned long arg2)
 	case ARCH_SHSTK_ENABLE:
 	case ARCH_SHSTK_DISABLE:
 	case ARCH_SHSTK_LOCK:
+	case ARCH_SHSTK_UNLOCK:
 		return shstk_prctl(task, option, arg2);
 	default:
 		ret = -EINVAL;
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) {
+		if (option == ARCH_SHSTK_UNLOCK && IS_ENABLED(CONFIG_CHECKPOINT_RESTORE)) {
+			task->thread.features_locked &= ~features;
+			return 0;
+		}
 		return -EINVAL;
+	}
 
 	/* Do not allow to change locked features */
 	if (features & task->thread.features_locked)