MIME-Version: 1.0
References: <20230227222957.24501-1-rick.p.edgecombe@intel.com>
 <20230227222957.24501-29-rick.p.edgecombe@intel.com> <ZAhjLAIm91rJ2Lpr@zn.tnic>
 <9e00b2a3d988f7b24d274a108d31f5f0096eeaae.camel@intel.com>
 <20230309125739.GCZAnXw5T1dfzwtqh8@fat_crate.local> <a4dd415ac908450b09b9abbd4421a9132b3c34cc.camel@intel.com>
 <20230309235152.GBZApxGNnXLvkGXCet@fat_crate.local> <e83ee9fc1a6e98cab62b681de7209598394df911.camel@intel.com>
In-Reply-To: <e83ee9fc1a6e98cab62b681de7209598394df911.camel@intel.com>
From:   "H.J. Lu" <hjl.tools@gmail.com>
Date:   Thu, 9 Mar 2023 18:03:49 -0800
Message-ID: <CAMe9rOrK2d6+Y_Xb+NUW4i+GWRbX+mGx+mJLwnEAB4hvsQ_eiw@mail.gmail.com>
Subject: Re: [PATCH v7 28/41] x86: Introduce userspace API for shadow stack
To:     "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>
Cc:     "bp@alien8.de" <bp@alien8.de>,
        "joao@overdrivepizza.com" <joao@overdrivepizza.com>,
        "david@redhat.com" <david@redhat.com>,
        "bsingharora@gmail.com" <bsingharora@gmail.com>,
        "hpa@zytor.com" <hpa@zytor.com>,
        "Syromiatnikov, Eugene" <esyr@redhat.com>,
        "peterz@infradead.org" <peterz@infradead.org>,
        "rdunlap@infradead.org" <rdunlap@infradead.org>,
        "keescook@chromium.org" <keescook@chromium.org>,
        "Eranian, Stephane" <eranian@google.com>,
        "kirill.shutemov@linux.intel.com" <kirill.shutemov@linux.intel.com>,
        "dave.hansen@linux.intel.com" <dave.hansen@linux.intel.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "fweimer@redhat.com" <fweimer@redhat.com>,
        "nadav.amit@gmail.com" <nadav.amit@gmail.com>,
        "jannh@google.com" <jannh@google.com>,
        "dethoma@microsoft.com" <dethoma@microsoft.com>,
        "kcc@google.com" <kcc@google.com>,
        "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>,
        "pavel@ucw.cz" <pavel@ucw.cz>, "oleg@redhat.com" <oleg@redhat.com>,
        "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "Lutomirski, Andy" <luto@kernel.org>,
        "jamorris@linux.microsoft.com" <jamorris@linux.microsoft.com>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "tglx@linutronix.de" <tglx@linutronix.de>,
        "Schimpe, Christina" <christina.schimpe@intel.com>,
        "mike.kravetz@oracle.com" <mike.kravetz@oracle.com>,
        "debug@rivosinc.com" <debug@rivosinc.com>,
        "Yang, Weijiang" <weijiang.yang@intel.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "andrew.cooper3@citrix.com" <andrew.cooper3@citrix.com>,
        "john.allen@amd.com" <john.allen@amd.com>,
        "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>,
        "rppt@kernel.org" <rppt@kernel.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
        "gorcunov@gmail.com" <gorcunov@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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.


-- 
H.J.