Re: [PATCH v5 19/39] mm: Fixup places that call pte_mkwrite() directly

[Edgecombe, Rick P]

On Mon, 2023-02-13 at 22:10 -0800, Deepak Gupta wrote:
> On Tue, Feb 14, 2023 at 01:07:24AM +0000, Edgecombe, Rick P wrote:
> > On Mon, 2023-02-13 at 16:09 -0800, Deepak Gupta wrote:
> > > Since I've a general question on outcome of discussion of how to
> > > handle
> > > `pte_mkwrite`, so I am top posting.
> > > 
> > > I have posted patches yesterday targeting riscv zisslpcfi
> > > extension.
> > > 
> > 
> > 
https://lore.kernel.org/lkml/20230213045351.3945824-1-debug@xxxxxxxxxxxx/
> > > 
> > > Since there're similarities in extension(s), patches have
> > > similarity
> > > too.
> > > One of the similarity was updating `maybe_mkwrite`. I was asked
> > > (by
> > > dhildenb
> > > on my patch #11) to look at x86 approach on how to approach this
> > > so
> > > that
> > > core-mm approach fits multiple architectures along with the need
> > > to
> > > update `pte_mkwrite` to consume vma flags.
> > > In x86 CET patch series, I see that locations where `pte_mkwrite`
> > > is
> > > invoked are updated to check for shadow stack vma and not
> > > necessarily
> > > `pte_mkwrite` itself is updated to consume vma flags. Let me know
> > > if
> > > my
> > > understanding is correct and that's the current direction (to
> > > update
> > > call sites for vma check where `pte_mkwrite` is invoked)
> > > 
> > > Being said that as I've mentioned in my patch series that
> > > there're
> > > similarities between x86, arm and now riscv for implementing
> > > shadow
> > > stack
> > > and indirect branch tracking, overall it'll be a good thing if we
> > > can
> > > collaborate and come up with common bits.
> > 
> > Oh interesting. I've made the changes to have pte_mkwrite() take a
> > VMA.
> > It seems to work pretty well with the core MM code, but I'm letting
> > 0-
> > day chew on it for a bit because it touched so many arch's. I'll
> > include you when I send it out, hopefully later this week.
> 
> Thanks.
> > 
> > From just a quick look, I see some design aspects that have been
> > problematic on the x86 implementation.
> > 
> > There was something like PROT_SHADOW_STACK before, but there were
> > two
> > problems:
> > 1. Writable windows while provisioning restore tokens (maybe this
> > is
> > just an x86 thing)
> > 2. Adding guard pages when a shadow stack was mprotect()ed to
> > change it
> > from writable to shadow stack. Again this might be an x86 need,
> > since
> > it needed to have it writable to add a restore token, and the guard
> > pages help with security.
> 
> I've not seen your earlier patch but I am assuming when you say
> window you
> mean that shadow stack was open to regular stores (or I may be
> missing
> something here)
> 
> I am wondering if mapping it as shadow stack (instead of having
> temporary
> writeable mapping) and using `wruss` was an option to put the token
> or
> you wanted to avoid it?
> 
> And yes on riscv, architecture itself doesn't define token or its
> format.
> Since it's RISC, software can define the token format and thus can
> use
> either `sspush` or `ssamoswap` to put a token on `shadow stack`
> virtual
> memory.

With WRSS a token could be created via software, but x86 shadow stack
includes instructions to create and switch to tokens in limited ways
(RSTORSSP, SAVEPREVSSP), where WRSS lets you write anything. These
other instructions are enough for glibc, except for writing a restore
token on a brand new shadow stack.

So WRSS is made optional since it weakens the protection of the shadow
stack. Some apps may prefer to use it to do exotic things, but the
glibc implementation didn't require it.

> 
> > 
> > So instead this series creates a map_shadow_stack syscall that maps
> > a
> > shadow stack and writes the token from the kernel side. Then
> > mprotect()
> > is prevented from making shadow stack's conventionally writable.
> > 
> > another difference is enabling shadow stack based on elf header
> > bits
> > instead of the arch_prctl()s. See the history and reasoning here
> > (section "Switch Enabling Interface"):
> > 
> > 
https://lore.kernel.org/lkml/20220130211838.8382-1-rick.p.edgecombe@xxxxxxxxx/
> > 
> > Not sure if those two issues would be problems on riscv or not.
> 
> Apart from mapping and window issue that you mentioned, I couldn't
> understand on why elf header bit is an issue only in this case for
> x86
> shadow stack and not an issue for let's say aarch64. I can see that
> aarch64 pretty much uses elf header bit for BTI. Eventually indirect
> branch tracking also needs to be enabled which is analogous to BTI.

Well for one, we had to deal with those old glibc's. But doesn't BTI
text need to be mapped with a special PROT as well? So it doesn't just
turn on enforcement automatically if it detects the elf bit.

> 
> BTW eventually riscv binaries plan to use `.riscv.attributes` section
> in riscv elf binary instead of `.gnu.note.property`. So I am hoping
> that
> part will go into arch specific code of elf parsing for riscv and
> will be
> contained.
> 
> > 
> > For sharing the prctl() interface. The other thing is that x86 also
> > has
> > this "wrss" instruction that can be enabled with shadow stack. The
> > current arch_prctl() interface supports both. I'm thinking it's
> > probably a pretty arch-specific thing.
> 
> yes ability to perform writes on shadow stack absolutely are
> prevented on
> x86. So enabling that should be a arch specific prctl.
> 
> > 
> > ABI-wise, are you planning to automatically allocate shadow stacks
> > for
> > new tasks? If the ABI is completely different it might be best to
> > not
> > share user interfaces. But also, I wonder why is it different.
> 
> Yes as of now planning both:
> - allocate shadow stack for new task based on elf header
> - task can create them using `prctls` (from glibc)
> 
> And yes `fork` will get the all cfi properties (shdow stack and
> branch tracking)
> from parent.

Have you looked at a riscv libc implementation yet? For unifying ABI I
think that might be best interface to target, for app developers. Then
each arch can implement enough kernel functionality to support libc
(for example map_shadow_stack).