Re: [PATCH v7 28/41] x86: Introduce userspace API for shadow stack

[Edgecombe, Rick P]

On Wed, 2023-03-08 at 11:27 +0100, Borislav Petkov wrote:
> On Mon, Feb 27, 2023 at 02:29:44PM -0800, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> > 
> > Add three new arch_prctl() handles:
> > 
> >  - ARCH_SHSTK_ENABLE/DISABLE enables or disables the specified
> >    feature. Returns 0 on success or an error.
> 
> "... or a negative value on error."

Sure.

> 
> >  - ARCH_SHSTK_LOCK prevents future disabling or enabling of the
> >    specified feature. Returns 0 on success or an error
> 
> ditto.
> 
> What is the use case of the feature locking?
> 
> I'm under the simple assumption that once shstk is enabled for an
> app,
> it remains so. I guess my question is rather, what's the use case for
> enabling shadow stack and then disabling it later for an app...?

This would be for things like the "permissive mode", where glibc
determines that it has to do something like dlopen() an unsupporting
DSO much later.

But being able to late lock the features is required for the working
behavior of glibc as well. Glibc enables shadow stack very early, then
disables it later if it finds that any of the normal dynamic libraries
don't support it. It only locks shadow stack after this point even in
non-permissive mode.

The selftest also does a lot of enabling and disabling.

> 
> > The features are handled per-thread and inherited over
> > fork(2)/clone(2),
> > but reset on exec().
> > 
> > This is preparation patch. It does not implement any features.
> 
> That belongs under the "---" line I guess.

Oh, yes.

> 
> > 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>
> > Reviewed-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > [tweaked with feedback from tglx]
> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> > 
> > ---
> > v4:
> >  - Remove references to CET and replace with shadow stack (Peterz)
> > 
> > v3:
> >  - Move shstk.c Makefile changes earlier (Kees)
> >  - Add #ifdef around features_locked and features (Kees)
> >  - Encapsulate features reset earlier in reset_thread_features() so
> >    features and features_locked are not referenced in code that
> > would be
> >    compiled !CONFIG_X86_USER_SHADOW_STACK. (Kees)
> >  - Fix typo in commit log (Kees)
> >  - Switch arch_prctl() numbers to avoid conflict with LAM
> > 
> > v2:
> >  - Only allow one enable/disable per call (tglx)
> >  - Return error code like a normal arch_prctl() (Alexander
> > Potapenko)
> >  - Make CET only (tglx)
> > ---
> >  arch/x86/include/asm/processor.h  |  6 +++++
> >  arch/x86/include/asm/shstk.h      | 21 +++++++++++++++
> >  arch/x86/include/uapi/asm/prctl.h |  6 +++++
> >  arch/x86/kernel/Makefile          |  2 ++
> >  arch/x86/kernel/process_64.c      |  7 ++++-
> >  arch/x86/kernel/shstk.c           | 44
> > +++++++++++++++++++++++++++++++
> >  6 files changed, 85 insertions(+), 1 deletion(-)
> >  create mode 100644 arch/x86/include/asm/shstk.h
> >  create mode 100644 arch/x86/kernel/shstk.c
> 
> ...
> 
> > +long shstk_prctl(struct task_struct *task, int option, unsigned
> > long features)
> > +{
> > +	if (option == ARCH_SHSTK_LOCK) {
> > +		task->thread.features_locked |= features;
> > +		return 0;
> > +	}
> > +
> > +	/* Don't allow via ptrace */
> > +	if (task != current)
> > +		return -EINVAL;
> > +
> > +	/* Do not allow to change locked features */
> > +	if (features & task->thread.features_locked)
> > +		return -EPERM;
> > +
> > +	/* Only support enabling/disabling one feature at a time. */
> > +	if (hweight_long(features) > 1)
> > +		return -EINVAL;
> > +
> > +	if (option == ARCH_SHSTK_DISABLE) {
> > +		return -EINVAL;
> > +	}
> 
> {} braces left over from some previous version. Can go now.
> 

This was intentional, but I wasn't sure on it. It makes the diff
cleaner in later patches, is the reason.