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

  From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>

Add three new arch_prctl() handles:

 - ARCH_SHSTK_ENABLE/DISABLE enables or disables the specified
   feature. Returns 0 on success or an error.

 - ARCH_SHSTK_LOCK prevents future disabling or enabling of the
   specified feature. Returns 0 on success or an error

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.

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: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
[tweaked with feedback from tglx]
Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

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

On Mon, Feb 27, 2023 at 02:29:44PM -0800, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> 
> 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."

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

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

> 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: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [tweaked with feedback from tglx]
> Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> 
> ---
> 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.
  

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@linux.intel.com>
> > 
> > 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@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: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > [tweaked with feedback from tglx]
> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > 
> > ---
> > 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.
  

On Wed, Mar 08, 2023 at 11:32:36PM +0000, Edgecombe, Rick P wrote:
> 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.

So this all sounds weird. Especially from a user point of view.

Now let's imagine there's a Linux user called Boris and he goes and buys
a CPU which supports shadow stack, gets a distro which has shadow stack
enabled. All good.

Now, at some point he loads a program which pulls in an old library
which hasn't been enabled for shadow stack yet.

In the name of not breaking stuff, his glibc is configured in permissive
mode by default so that program loads and shadow stack for it is
disabled.

And Boris doesn't even know and continues on his merry way thinking that
he has all that cool ROP protection.

So where is the knob that says, "disable permissive mode"?

Or at least where does the user get a warning saying, "hey, this app
doesn't do shadow stack and we disabled it for ya so that it can still
work"?

Or am I way off?

I hope you're catching my drift. Because if there's no enforcement of
shstk and we do this permissive mode by default, this whole overhead is
just a unnecessary nuisance...

But maybe that'll come later and I should keep going through the set...

Thx.
  

On Thu, 2023-03-09 at 13:57 +0100, Borislav Petkov wrote:
> So this all sounds weird. Especially from a user point of view.
> 
> Now let's imagine there's a Linux user called Boris and he goes and
> buys
> a CPU which supports shadow stack, gets a distro which has shadow
> stack
> enabled. All good.
> 
> Now, at some point he loads a program which pulls in an old library
> which hasn't been enabled for shadow stack yet.
> 
> In the name of not breaking stuff, his glibc is configured in
> permissive
> mode by default so that program loads and shadow stack for it is
> disabled.
> 
> And Boris doesn't even know and continues on his merry way thinking
> that
> he has all that cool ROP protection.

There is a proc that shows if shadow stack is enabled in a thread. It
does indeed come later in the series.

> 
> So where is the knob that says, "disable permissive mode"?

glibc has an environment variable that can change the loader's
behavior. There is also a compile time config for the default mode. But
this "permissive mode" is a glibc thing. The kernel doesn't implement
it per-se, just provides building blocks.

> 
> Or at least where does the user get a warning saying, "hey, this app
> doesn't do shadow stack and we disabled it for ya so that it can
> still
> work"?
> 
> Or am I way off?

I don't think so. The whole "when to enable shadow stack" question is
thornier than it might seem though, and what we have here is based on
some trade offs in the details.

> 
> I hope you're catching my drift. Because if there's no enforcement of
> shstk and we do this permissive mode by default, this whole overhead
> is
> just a unnecessary nuisance...

In the existing glibc patches, and this is highly related to glibc
behavior because the decisions around enabling and locking have been
pushed there, there are two reasons why shadow stack would get disabled
on an supporting executable after it gets enabled.
1. An executable is loaded and one of the shared objects (the ones that
come out of ldd) does not support shadow stack
2. An executable is loaded in permissive mode, and much later during
execution dlopen()s a DSO that does not support shadow stack.

One of the challenges with enabling shadow stack is you only start
recording the shadow stack history when you enable it. If you enable it
at some point, and then return from that function you underflow the
shadow stack and get a violation. So if the shadow stack will be
locked, it has to be enabled at the earliest point it might return to
at some point (for example after returning from main()).

So in 1, the existing logic of glibc is to enable shadow stack at the
very beginning of the loader. Then go through the whole loading/linking
process. If problems are found, disable shadow stack. If no problems
are found, then lock it.

I've wondered if this super early glibc enabling behavior is really
needed and if they could enable it after processing the linked
libraries in the elf. Then save the work of enabling and disabling
shadow stack for situations that don't support it. To me this is the
big wart in the whole thing, but I don't think the kernel can help
resolve it. If glibc can enable later, then we can combine the locking
and enabling into a single operation. But it only saves a syscall and
it might prevent some other libc that needs to do things like glibc
does currently, from being able to make it work at all.

In 2, the enabling happens like normal and the locking is skipped, so
that shadow stack can be enabled during a dlopen(). But glibc
permissive mode promises more than it delivers. Since it can only
disable shadow stack per-thread, it leaves the other threads enabled.
Making a working permissive mode is sort of an unsolved problem. There
are some proposals to make it work in just userspace, and some that
would need additional kernel support. If you are interested I can go
into why per-process disabling is not straightforward.

So the locking is needed for the basic support in 1 and the weak
permissive mode in 2 uses it. I am considering this series to support
1, but people may end up using 2 to get some permissive-ness. In
general the idea of this API is to push the enabling decisions into
userspace because that is where the information for making the decision
is known. We previously tried to add some batch operations to improve
the performance, but tglx had suggested to start with something simple.
So we end up with this simple composable API.
  

On Thu, Mar 09, 2023 at 04:56:37PM +0000, Edgecombe, Rick P wrote:
> There is a proc that shows if shadow stack is enabled in a thread. It
> does indeed come later in the series.

Not good enough:

1. buried somewhere in proc where no one knows about it

2. it is per thread so user needs to grep *all*

>  ... We previously tried to add some batch operations to improve the
>  performance, but tglx had suggested to start with something simple.
>  So we end up with this simple composable API.

I agree with starting simple and thanks for explaining this in detail.

TBH, though, it already sounds like a mess to me. I guess a mess we'll
have to deal with because there will always be this case of some
shared object/lib not being enabled for shstk because of raisins.

And TBH #2, I would've done it even simpler: if some shared object can't
do shadow stack, we disable it for the whole process. I mean, what's the
point? Only some of the stack is shadowed so an attacker could find
a way to keep the process perhaps run this shstk-unsupporting shared
object more/longer and ROP its way around the system.

But I tend to oversimplify things sometimes so...

What I'd like to have, though, is a kernel cmdline param which disables
permissive mode and userspace can't do anything about it. So that once
you boot your kernel, you can know that everything that runs on the
machine has shstk and is properly protected.

Also, it'll allow for faster fixing of all those shared objects to use
shstk by way of political pressure.

Thx.
  

+Joao regarding mixed mode designs

On Fri, 2023-03-10 at 00:51 +0100, Borislav Petkov wrote:
> On Thu, Mar 09, 2023 at 04:56:37PM +0000, Edgecombe, Rick P wrote:
> > There is a proc that shows if shadow stack is enabled in a thread.
> > It
> > does indeed come later in the series.
> 
> Not good enough:
> 
> 1. buried somewhere in proc where no one knows about it
> 
> 2. it is per thread so user needs to grep *all*

See "x86: Expose thread features in /proc/$PID/status" for the patch.
We could emit something in dmesg I guess? The logic would be:
 - Record the presence of elf SHSTK bit on exec
 - On shadow stack disable, if it had the elf bit, pr_info("bad!")

> 
> >   ... We previously tried to add some batch operations to improve
> > the
> >   performance, but tglx had suggested to start with something
> > simple.
> >   So we end up with this simple composable API.
> 
> I agree with starting simple and thanks for explaining this in
> detail.
> 
> TBH, though, it already sounds like a mess to me. I guess a mess
> we'll
> have to deal with because there will always be this case of some
> shared object/lib not being enabled for shstk because of raisins.

The compatibility problems are totally the mess in this whole thing.
When you try to look at a "permissive" mode that actually works it gets
even more complex. Joao and I have been banging our heads on that
problem for months.

But there are some expected users of this that say: we compile and
check our known set of binaries, we won't get any surprises. So it's
more of a distro problem.

> 
> And TBH #2, I would've done it even simpler: if some shared object
> can't
> do shadow stack, we disable it for the whole process. I mean, what's
> the
> point? 

You mean a late loaded dlopen()ed DSO? The enabling logic can't know
this will happen ahead of time.

If you mean if the shared objects in the elf all support shadow stack,
then this is what happens. The complication is that the loader wants to
enable shadow stack before it has checked the elf libs so it doesn't
underflow the shadow stack when it returns from the function that does
this checking.

So it does:
1. Enable shadow stack
2. Call elf libs checking functions
3. If all good, lock shadow stack. Else, disable shadow stack.
4. Return from elf checking functions and if shstk is enabled, don't
underflow because it was enabled in step 1 and we have return addresses
from 2 on the shadow stack

I'm wondering if this can't be improved in glibc to look like:
1. Check elf libs, and record it somewhere
2. Wait until just the right spot
3. If all good, enable and lock shadow stack.

But it depends on the loader code design which I don't know well
enough.

> Only some of the stack is shadowed so an attacker could find
> a way to keep the process perhaps run this shstk-unsupporting shared
> object more/longer and ROP its way around the system.

I hope non-permissive mode is the standard usage eventually.

> 
> But I tend to oversimplify things sometimes so...
> 
> What I'd like to have, though, is a kernel cmdline param which
> disables
> permissive mode and userspace can't do anything about it. So that
> once
> you boot your kernel, you can know that everything that runs on the
> machine has shstk and is properly protected.

Szabolcs Nagy was commenting something similar in another thread, for
supporting kernel enforced security policies. I think the way to do it
would have the kernel detect the the elf bit itself (like it used to)
and enable shadow stack on exec. If you can't rely on userspace to call
in to enable it, it's not clear at what point the kernel should check
that it did.

But then if you trigger off of the elf bit in the kernel, you get all
the regression issues of the old glibcs at that point. But it is
already an "I don't care if I crash" mode, so...

I think if you trust your libc, glibc could implement this in userspace
too. It would be useful even as as testing override.

> 
> Also, it'll allow for faster fixing of all those shared objects to
> use
> shstk by way of political pressure.
  

On Thu, Mar 9, 2023 at 5:13 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> +Joao regarding mixed mode designs
>
> On Fri, 2023-03-10 at 00:51 +0100, Borislav Petkov wrote:
> > On Thu, Mar 09, 2023 at 04:56:37PM +0000, Edgecombe, Rick P wrote:
> > > There is a proc that shows if shadow stack is enabled in a thread.
> > > It
> > > does indeed come later in the series.
> >
> > Not good enough:
> >
> > 1. buried somewhere in proc where no one knows about it
> >
> > 2. it is per thread so user needs to grep *all*
>
> See "x86: Expose thread features in /proc/$PID/status" for the patch.
> We could emit something in dmesg I guess? The logic would be:
>  - Record the presence of elf SHSTK bit on exec
>  - On shadow stack disable, if it had the elf bit, pr_info("bad!")
>
> >
> > >   ... We previously tried to add some batch operations to improve
> > > the
> > >   performance, but tglx had suggested to start with something
> > > simple.
> > >   So we end up with this simple composable API.
> >
> > I agree with starting simple and thanks for explaining this in
> > detail.
> >
> > TBH, though, it already sounds like a mess to me. I guess a mess
> > we'll
> > have to deal with because there will always be this case of some
> > shared object/lib not being enabled for shstk because of raisins.
>
> The compatibility problems are totally the mess in this whole thing.
> When you try to look at a "permissive" mode that actually works it gets
> even more complex. Joao and I have been banging our heads on that
> problem for months.
>
> But there are some expected users of this that say: we compile and
> check our known set of binaries, we won't get any surprises. So it's
> more of a distro problem.
>
> >
> > And TBH #2, I would've done it even simpler: if some shared object
> > can't
> > do shadow stack, we disable it for the whole process. I mean, what's
> > the
> > point?
>
> You mean a late loaded dlopen()ed DSO? The enabling logic can't know
> this will happen ahead of time.
>
> If you mean if the shared objects in the elf all support shadow stack,
> then this is what happens. The complication is that the loader wants to
> enable shadow stack before it has checked the elf libs so it doesn't
> underflow the shadow stack when it returns from the function that does
> this checking.
>
> So it does:
> 1. Enable shadow stack
> 2. Call elf libs checking functions
> 3. If all good, lock shadow stack. Else, disable shadow stack.
> 4. Return from elf checking functions and if shstk is enabled, don't
> underflow because it was enabled in step 1 and we have return addresses
> from 2 on the shadow stack
>
> I'm wondering if this can't be improved in glibc to look like:
> 1. Check elf libs, and record it somewhere
> 2. Wait until just the right spot
> 3. If all good, enable and lock shadow stack.

I will try it out.

> But it depends on the loader code design which I don't know well
> enough.
>
> > Only some of the stack is shadowed so an attacker could find
> > a way to keep the process perhaps run this shstk-unsupporting shared
> > object more/longer and ROP its way around the system.
>
> I hope non-permissive mode is the standard usage eventually.
>
> >
> > But I tend to oversimplify things sometimes so...
> >
> > What I'd like to have, though, is a kernel cmdline param which
> > disables
> > permissive mode and userspace can't do anything about it. So that
> > once
> > you boot your kernel, you can know that everything that runs on the
> > machine has shstk and is properly protected.
>
> Szabolcs Nagy was commenting something similar in another thread, for
> supporting kernel enforced security policies. I think the way to do it
> would have the kernel detect the the elf bit itself (like it used to)
> and enable shadow stack on exec. If you can't rely on userspace to call
> in to enable it, it's not clear at what point the kernel should check
> that it did.
>
> But then if you trigger off of the elf bit in the kernel, you get all
> the regression issues of the old glibcs at that point. But it is
> already an "I don't care if I crash" mode, so...
>
> I think if you trust your libc, glibc could implement this in userspace
> too. It would be useful even as as testing override.
>
> >
> > Also, it'll allow for faster fixing of all those shared objects to
> > use
> > shstk by way of political pressure.
  

On Fri, Mar 10, 2023 at 01:13:42AM +0000, Edgecombe, Rick P wrote:
> See "x86: Expose thread features in /proc/$PID/status" for the patch.
> We could emit something in dmesg I guess? The logic would be:

dmesg is just flaky: ring buffer can get overwritten, users don't see
it, ...

> The compatibility problems are totally the mess in this whole thing.
> When you try to look at a "permissive" mode that actually works it gets
> even more complex. Joao and I have been banging our heads on that
> problem for months.

Oh yeah, I'm soo NOT jealous. :-\

> But there are some expected users of this that say: we compile and
> check our known set of binaries, we won't get any surprises. So it's
> more of a distro problem.

I'm guessing what will happen here is that distros will gradually enable
shstk and once it is ubiquitous, there will be no reason to disable it
at all.

> You mean a late loaded dlopen()ed DSO? The enabling logic can't know
> this will happen ahead of time.

No, I meant the case where you start with shstk enabled and later
disable it when some lib does not support it.

From now on that whole process is marked as "cannot use shstk anymore"
and any other shared object that tries to use shstk simply doesn't get
it.

But meh, this makes the situation even more convoluted as the stuff that
has loaded before the first shstk-not-supporting lib, already uses
shstk.

So you have half and half.

What a mess.

> I hope non-permissive mode is the standard usage eventually.

Yah.

> I think if you trust your libc, glibc could implement this in userspace
> too. It would be useful even as as testing override.

No, you cannot trust any userspace. And there are other libc's beside
glibc.

This should be a kernel parameter. I'm not saying we should do it now
but we should do it at some point.

So that user Boris again, he installs his new shiny distro, he checks
that all the use cases and software he uses there is already
shstk-enabled and then he goes and builds the kernel with

	CONFIG_X86_USER_SHADOW_STACK_STRICT=y

or supplies a cmdline param and from now on, nothing can run without
shstk. No checking, no trusting, no nothing.

We fail any thread creation which doesn't init shstk.

Thx.
  

On Thu, Mar 9, 2023 at 6:03 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Mar 9, 2023 at 5:13 PM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> >
> > +Joao regarding mixed mode designs
> >
> > On Fri, 2023-03-10 at 00:51 +0100, Borislav Petkov wrote:
> > > On Thu, Mar 09, 2023 at 04:56:37PM +0000, Edgecombe, Rick P wrote:
> > > > There is a proc that shows if shadow stack is enabled in a thread.
> > > > It
> > > > does indeed come later in the series.
> > >
> > > Not good enough:
> > >
> > > 1. buried somewhere in proc where no one knows about it
> > >
> > > 2. it is per thread so user needs to grep *all*
> >
> > See "x86: Expose thread features in /proc/$PID/status" for the patch.
> > We could emit something in dmesg I guess? The logic would be:
> >  - Record the presence of elf SHSTK bit on exec
> >  - On shadow stack disable, if it had the elf bit, pr_info("bad!")
> >
> > >
> > > >   ... We previously tried to add some batch operations to improve
> > > > the
> > > >   performance, but tglx had suggested to start with something
> > > > simple.
> > > >   So we end up with this simple composable API.
> > >
> > > I agree with starting simple and thanks for explaining this in
> > > detail.
> > >
> > > TBH, though, it already sounds like a mess to me. I guess a mess
> > > we'll
> > > have to deal with because there will always be this case of some
> > > shared object/lib not being enabled for shstk because of raisins.
> >
> > The compatibility problems are totally the mess in this whole thing.
> > When you try to look at a "permissive" mode that actually works it gets
> > even more complex. Joao and I have been banging our heads on that
> > problem for months.
> >
> > But there are some expected users of this that say: we compile and
> > check our known set of binaries, we won't get any surprises. So it's
> > more of a distro problem.
> >
> > >
> > > And TBH #2, I would've done it even simpler: if some shared object
> > > can't
> > > do shadow stack, we disable it for the whole process. I mean, what's
> > > the
> > > point?
> >
> > You mean a late loaded dlopen()ed DSO? The enabling logic can't know
> > this will happen ahead of time.
> >
> > If you mean if the shared objects in the elf all support shadow stack,
> > then this is what happens. The complication is that the loader wants to
> > enable shadow stack before it has checked the elf libs so it doesn't
> > underflow the shadow stack when it returns from the function that does
> > this checking.
> >
> > So it does:
> > 1. Enable shadow stack
> > 2. Call elf libs checking functions
> > 3. If all good, lock shadow stack. Else, disable shadow stack.
> > 4. Return from elf checking functions and if shstk is enabled, don't
> > underflow because it was enabled in step 1 and we have return addresses
> > from 2 on the shadow stack
> >
> > I'm wondering if this can't be improved in glibc to look like:
> > 1. Check elf libs, and record it somewhere
> > 2. Wait until just the right spot
> > 3. If all good, enable and lock shadow stack.
>
> I will try it out.
>

Currently glibc enables shadow stack as early as possible.  There
are only a few places where a function call in glibc never returns.
We can enable shadow stack just before calling main.   There are
quite some code paths without shadow stack protection.   Is this
an issue?

H.J.
  

On Fri, 2023-03-10 at 12:00 -0800, H.J. Lu wrote:
> > > So it does:
> > > 1. Enable shadow stack
> > > 2. Call elf libs checking functions
> > > 3. If all good, lock shadow stack. Else, disable shadow stack.
> > > 4. Return from elf checking functions and if shstk is enabled,
> > > don't
> > > underflow because it was enabled in step 1 and we have return
> > > addresses
> > > from 2 on the shadow stack
> > > 
> > > I'm wondering if this can't be improved in glibc to look like:
> > > 1. Check elf libs, and record it somewhere
> > > 2. Wait until just the right spot
> > > 3. If all good, enable and lock shadow stack.
> > 
> > I will try it out.
> > 
> 
> Currently glibc enables shadow stack as early as possible.  There
> are only a few places where a function call in glibc never returns.
> We can enable shadow stack just before calling main.   There are
> quite some code paths without shadow stack protection.   Is this
> an issue?

Thanks for checking. Hmm, does the loader get attacked?
  

On Fri, Mar 10, 2023 at 12:27 PM Edgecombe, Rick P
<rick.p.edgecombe@intel.com> wrote:
>
> On Fri, 2023-03-10 at 12:00 -0800, H.J. Lu wrote:
> > > > So it does:
> > > > 1. Enable shadow stack
> > > > 2. Call elf libs checking functions
> > > > 3. If all good, lock shadow stack. Else, disable shadow stack.
> > > > 4. Return from elf checking functions and if shstk is enabled,
> > > > don't
> > > > underflow because it was enabled in step 1 and we have return
> > > > addresses
> > > > from 2 on the shadow stack
> > > >
> > > > I'm wondering if this can't be improved in glibc to look like:
> > > > 1. Check elf libs, and record it somewhere
> > > > 2. Wait until just the right spot
> > > > 3. If all good, enable and lock shadow stack.
> > >
> > > I will try it out.
> > >
> >
> > Currently glibc enables shadow stack as early as possible.  There
> > are only a few places where a function call in glibc never returns.
> > We can enable shadow stack just before calling main.   There are
> > quite some code paths without shadow stack protection.   Is this
> > an issue?
>
> Thanks for checking. Hmm, does the loader get attacked?

Not I know of.  But there are user codes from .init_array
and .preinit_array which are executed before main.   In theory,
an attack can happen before main.
  

On Fri, 2023-03-10 at 12:43 -0800, H.J. Lu wrote:
> On Fri, Mar 10, 2023 at 12:27 PM Edgecombe, Rick P
> <rick.p.edgecombe@intel.com> wrote:
> > 
> > On Fri, 2023-03-10 at 12:00 -0800, H.J. Lu wrote:
> > > > > So it does:
> > > > > 1. Enable shadow stack
> > > > > 2. Call elf libs checking functions
> > > > > 3. If all good, lock shadow stack. Else, disable shadow
> > > > > stack.
> > > > > 4. Return from elf checking functions and if shstk is
> > > > > enabled,
> > > > > don't
> > > > > underflow because it was enabled in step 1 and we have return
> > > > > addresses
> > > > > from 2 on the shadow stack
> > > > > 
> > > > > I'm wondering if this can't be improved in glibc to look
> > > > > like:
> > > > > 1. Check elf libs, and record it somewhere
> > > > > 2. Wait until just the right spot
> > > > > 3. If all good, enable and lock shadow stack.
> > > > 
> > > > I will try it out.
> > > > 
> > > 
> > > Currently glibc enables shadow stack as early as possible.  There
> > > are only a few places where a function call in glibc never
> > > returns.
> > > We can enable shadow stack just before calling main.   There are
> > > quite some code paths without shadow stack protection.   Is this
> > > an issue?
> > 
> > Thanks for checking. Hmm, does the loader get attacked?
> 
> Not I know of.  But there are user codes from .init_array
> and .preinit_array which are executed before main.   In theory,
> an attack can happen before main.

Hmm, it would be nice to not add any startup overhead to non-shadow
stack binaries. I guess it's a tradeoff. Might be worth asking around.

But you can't just enable shadow stack before any user code? It has to
go something like?
1. Execute init codes
2. Check elf libs
3. Enable SHSTK

Or what if you just did the enable-disable dance if the execing binary
itself has shadow stack. If it doesn't have shadow stack, the elf libs
won't change the decision.