Message-ID: <e86ac1468767b8cf7f02c64bdb26b2ed951aa992.camel@intel.com>
Date:   Sat, 12 Feb 2022 00:10:25 +0000
From:   "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To:     "Lutomirski, Andy" <luto@...nel.org>,
        "Hansen, Dave" <dave.hansen@...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>,
        "Yu, Yu-cheng" <yu-cheng.yu@...el.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>,
        "fweimer@...hat.com" <fweimer@...hat.com>,
        "nadav.amit@...il.com" <nadav.amit@...il.com>,
        "jannh@...gle.com" <jannh@...gle.com>,
        "kcc@...gle.com" <kcc@...gle.com>,
        "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
        "bp@...en8.de" <bp@...en8.de>, "oleg@...hat.com" <oleg@...hat.com>,
        "hjl.tools@...il.com" <hjl.tools@...il.com>,
        "Yang, Weijiang" <weijiang.yang@...el.com>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "pavel@....cz" <pavel@....cz>, "arnd@...db.de" <arnd@...db.de>,
        "Moreira, Joao" <joao.moreira@...el.com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mike.kravetz@...cle.com" <mike.kravetz@...cle.com>,
        "x86@...nel.org" <x86@...nel.org>,
        "dave.martin@....com" <dave.martin@....com>,
        "john.allen@....com" <john.allen@....com>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "Shankar, Ravi V" <ravi.v.shankar@...el.com>,
        "corbet@....net" <corbet@....net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "gorcunov@...il.com" <gorcunov@...il.com>
Subject: Re: [PATCH 18/35] mm: Add guard pages around a shadow stack.

On Fri, 2022-02-11 at 09:54 -0800, Andy Lutomirski wrote:
> > Like just using VM_SHADOW_STACK | VM_GROWSDOWN to get a regular
> > stack
> > sized gap? I think it could work. It also simplifies the mm-
> > >stack_vm
> > accounting.
> 
> Seems not crazy.  Do we want automatically growing shadow stacks?  I 
> don't really like the historical unix behavior where the main thread
> has 
> a sort-of-infinite stack and every other thread has a fixed stack.

Ah! I was scratching my head why glibc did that - it's historical. Yea,
probably it's not needed and adding strange behavior.

> 
> > 
> > It would no longer get a gap at the end though. I don't think it's
> > needed.
> > 
> 
> I may have missed something about the oddball way the mm code works,
> but 
> it seems if you have a gap at one end of every shadow stack, you 
> automatically have a gap at the other end.

Right, we only need one, and this patch added a gap on both ends.

Per Andy's comment about the "oddball way" the mm code does the gaps -
The previous version of this (PROT_SHADOW_STACK) had an issue where if
you started with writable memory, then mprotect() with
PROT_SHADOW_STACK, the internal rb tree would get confused over the
sudden appearance of a gap. This new version follows closer to how
MAP_STACK avoids the problem I saw. But the way these guard gaps work
seem to barely avoid problems when you do things like split the vma by
mprotect()ing the middle of one. I wasn't sure if it's worth a
refactor. I guess the solution is quite old and there hasn't been
problems. I'm not even sure what the change would be, but it does feel
like adding to something fragile. Maybe shadow stack should just place
a guard page manually and not add any special shadow stack logic to the
mm code...

Other than that I'm inclined to remove the end gap and justify this
patch better in the commit log. Something like this (borrowing some
from Dave's comments):

	The architecture of shadow stack constrains the ability of 
	userspace to move the shadow stack pointer (ssp) in order to 
	prevent corrupting or switching to other shadow stacks. The 
	RSTORSSP can move the spp to different shadow stacks, but it 
	requires a specially placed token in order to switch shadow 
	stacks. However, the architecture does not prevent 
	incrementing or decrementing shadow stack pointer to wander 
	onto an adjacent shadow stack. To prevent this in software, 
	enforce guard pages at the beginning of shadow stack vmas, such
	that t
here will always be a gap between adjacent shadow stacks.

	Make the gap big enough so that no userspace ssp changing 
	operations (besides RSTORSSP), can move the ssp from one stack 
	to the next. The ssp can increment or decrement by CALL, RET 
	and INCSSP. CALL and RET can move the ssp by a maximum of 8 
	bytes, at which point the shadow stack would be accessed.

        The INCSSP instruction can also increment the shadow stack 
	pointer. It is the shadow stack analog of an instruction like:

                addq    $0x80, %rsp

        However, there is one important difference between an ADD on
        %rsp and INCSSP. In addition to modifying SSP, INCSSP also
        reads from the memory of the first and last elements that were
        "popped". It can be thought of as acting like this:

        READ_ONCE(ssp);       // read+discard top element on stack
        ssp += nr_to_pop * 8; // move the shadow stack
        READ_ONCE(ssp-8);     // read+discard last popped stack element

	The maximum distance INCSSP can move the ssp is 2040 bytes, 
	before it would read the memory. There for a single page gap 
	will be enough to prevent any operation from shifting the ssp 
	to an adjacent gap, before the shadow stack would be read and 
	cause a fault.

	This could be accomplished by using VM_GROWSDOWN, but this has 
	two downsides.
	   1. VM_GROWSDOWN will have a 1MB gap which is on the large 
	      side for 32 bit address spaces
	   2. The behavior would allow shadow stack's to grow, which 
	      is unneeded and adds a strange difference to how most 
	      regular stacks work.

Powered by blists - more mailing lists