diff mbox series

[v3,36/37] x86/cet/shstk: Add ARCH_CET_UNLOCK

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

Commit Message

Edgecombe, Rick P Nov. 4, 2022, 10:36 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 CET 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>
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>
---

v3:
 - Depend on CONFIG_CHECKPOINT_RESTORE (Kees)

 Documentation/x86/cet.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

Peter Zijlstra Nov. 15, 2022, 2:47 p.m. UTC | #1 
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.

> +	if (task != current) {
> +		if (option == ARCH_CET_UNLOCK && IS_ENABLED(CONFIG_CHECKPOINT_RESTORE)) {

Why make this conditional on CRIU at all?

> +			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
>
Edgecombe, Rick P Nov. 15, 2022, 8:01 p.m. UTC | #2 
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
> >
Peter Zijlstra Nov. 15, 2022, 8:57 p.m. UTC | #3 
On Tue, Nov 15, 2022 at 08:01:12PM +0000, Edgecombe, Rick P wrote:
> > > +	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.

Well, distros will all have this stuff enabled no? So not much
protection in practise.
Dave Hansen Nov. 15, 2022, 9 p.m. UTC | #4 
On 11/15/22 12:57, Peter Zijlstra wrote:
> On Tue, Nov 15, 2022 at 08:01:12PM +0000, Edgecombe, Rick P wrote:
>>>> +	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.
> Well, distros will all have this stuff enabled no? So not much
> protection in practise.

Yeah, that's true for the distros.

But, I would imagine that our more paranoid friends like the ChromeOS
folks might appreciate this.
Peter Zijlstra Nov. 15, 2022, 9:21 p.m. UTC | #5 
On Tue, Nov 15, 2022 at 01:00:40PM -0800, Dave Hansen wrote:
> On 11/15/22 12:57, Peter Zijlstra wrote:
> > On Tue, Nov 15, 2022 at 08:01:12PM +0000, Edgecombe, Rick P wrote:
> >>>> +	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.
> > Well, distros will all have this stuff enabled no? So not much
> > protection in practise.
> 
> Yeah, that's true for the distros.
> 
> But, I would imagine that our more paranoid friends like the ChromeOS
> folks might appreciate this.

ptrace can modify text, I'm not sure what if anything we're protecting
against.
diff mbox series

Patch

diff --git a/Documentation/x86/cet.rst b/Documentation/x86/cet.rst
index b56811566531..f69cafb1feff 100644
--- a/Documentation/x86/cet.rst
+++ b/Documentation/x86/cet.rst
@@ -66,6 +66,10 @@  arch_prctl(ARCH_CET_LOCK, unsigned int 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_CET_UNLOCK, unsigned int features)
+    Unlock features. 'features' is a mask of all features to unlock. All
+    bits set are processed, unset bits are ignored.
+
 The return values are as following:
     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 5f1d3181e4a1..0c37fd0ad8d9 100644
--- a/arch/x86/include/uapi/asm/prctl.h
+++ b/arch/x86/include/uapi/asm/prctl.h
@@ -25,6 +25,7 @@ 
 #define ARCH_CET_ENABLE			0x5001
 #define ARCH_CET_DISABLE		0x5002
 #define ARCH_CET_LOCK			0x5003
+#define ARCH_CET_UNLOCK			0x5004
 
 /* ARCH_CET_ features bits */
 #define CET_SHSTK			(1ULL <<  0)
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 17fec059317c..03bc16c9cc19 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_CET_ENABLE:
 	case ARCH_CET_DISABLE:
 	case ARCH_CET_LOCK:
+	case ARCH_CET_UNLOCK:
 		return cet_prctl(task, option, arg2);
 	default:
 		ret = -EINVAL;
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 */
+	if (task != current) {
+		if (option == ARCH_CET_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)