Date:   Wed, 1 Jun 2022 11:06:08 +0300
From:   Mike Rapoport <rppt@...nel.org>
To:     "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc:     "bsingharora@...il.com" <bsingharora@...il.com>,
        "hpa@...or.com" <hpa@...or.com>,
        "Syromiatnikov, Eugene" <esyr@...hat.com>,
        "peterz@...radead.org" <peterz@...radead.org>,
        "rdunlap@...radead.org" <rdunlap@...radead.org>,
        "keescook@...omium.org" <keescook@...omium.org>,
        "0x7f454c46@...il.com" <0x7f454c46@...il.com>,
        "Eranian, Stephane" <eranian@...gle.com>,
        "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "adrian@...as.de" <adrian@...as.de>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "avagin@...il.com" <avagin@...il.com>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "pavel@....cz" <pavel@....cz>, "oleg@...hat.com" <oleg@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "Lutomirski, Andy" <luto@...nel.org>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "arnd@...db.de" <arnd@...db.de>,
        "Moreira, Joao" <joao.moreira@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "x86@...nel.org" <x86@...nel.org>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "john.allen@....com" <john.allen@....com>,
        "dave.martin@....com" <dave.martin@....com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Hansen, Dave" <dave.hansen@...el.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>
Subject: Re: [PATCH 00/35] Shadow stacks for userspace

On Tue, May 31, 2022 at 05:34:50PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2022-05-31 at 19:36 +0300, Mike Rapoport wrote:
> > > WRSS is a feature where you would usually want to lock it as
> > > disabled,
> > > but WRSS cannot be enabled if shadow stack is not enabled. Locking
> > > shadow stack and WRSS off together doesn't have any security
> > > benefits
> > > in theory. so I'm thinking glibc doesn't need to do this. The
> > > kernel
> > > could even refuse to lock WRSS without shadow stack being enabled.
> > > Could we avoid the extra ptrace functionality then?
> > 
> > What I see for is that a program can support shadow stack, glibc
> > enables
> > shadow stack, does not enable WRSS and than calls
> > 
> >         arch_prctl(ARCH_X86_FEATURE_LOCK,
> >                    LINUX_X86_FEATURE_SHSTK | LINUX_X86_FEATURE_WRSS);
> 
> I see the logic is glibc will lock SHSTK|IBT if either is enabled in
> the elf header. I guess that is why I didn't see the locking happening
> for me, because my manual enablement test doesn't have either set in
> the header.

The locking was quite a surprise for me when I moved from standalone test
to a system with CET-enabled glibc :)
 
> It can't see where that glibc knows about WRSS though...

Right, it was my mistake, as H.J. said glibc locks SHSTK and IBT.
 
> The glibc logic seems wrong to me also, because shadow stack or IBT
> could be force-disabled via glibc tunables. I don't see why the elf
> header bit should exclusively control the feature locking. Or why both
> should be locked if only one is in the header.
> 
> > 
> > so that WRSS cannot be re-enabled.
> > 
> > For the programs that do not support shadow stack, both SHSTK and
> > WRSS are
> > disabled, but still there is the same call to
> > arch_prctl(ARCH_X86_FEATURE_LOCK, ...) and then neither shadow stack
> > nor
> > WRSS can be enabled.
> > 
> > My original plan was to run CRIU with no shadow stack, enable shadow
> > stack
> > and WRSS in the restored tasks using arch_prct() and after the shadow
> > stack
> > contents is restored disable WRSS.
> > 
> > Obviously, this didn't work with glibc I have :)
> 
> Were you disabling shadow stack via glibc tunnable? Or was the elf
> header marked for IBT? If it was a plain old binary, the code looks to
> me like it should not lock any features.

I built criu as a plain old binary, there were no SHSTK or IBT markers. And
I've seen that there was a call to arch_prctl that locked the features as
disabled. 
 
> > On the bright side, having a ptrace call to unlock shadow stack and
> > wrss
> > allows running CRIU itself with shadow stack.
> 
> Yea, having something working is really great. My only hesitancy is
> that, per a discussion on the LAM patchset, we are going to make this
> enabling API CET only (same semantics for though). I suppose the
> locking API arch_prctl() could still be support other arch features,
> but it might be a second CET only regset. It's not the end of the
> world.

The support for CET in criu is anyway experimental for now, if the kernel
API will be slightly different in the end, we'll update criu.
The important things are the ability to control tracee shadow stack
from ptrace, the ability to map the shadow stack at fixed address and the
ability to control the features at least from ptrace.
As long as we have APIs that provide those, it should be Ok.
 
> I guess the other consideration is tieing CRIU to glibc peculiarities.
> Like even if we fix glibc, then CRIU may not work with some other libc
> or app that force disables for some weird reason. Is it supposed to be
> libc-agnostic?

Actually using the ptrace to control the CET features does not tie criu to
glibc. The current proposal for the arch_prctl() allows libc to lock CET
features and having a ptrace call to control the lock makes criu agnostic
to libc behaviour.

-- 
Sincerely yours,
Mike.

Powered by blists - more mailing lists