| Message ID | 20230227222957.24501-41-rick.p.edgecombe@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Shadow stacks for userspace | expand |
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...
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?
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 --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)