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

On Tue, Feb 14, 2023 at 10:24 AM Edgecombe, Rick P
[off-list ref] wrote:

On Mon, 2023-02-13 at 22:10 -0800, Deepak Gupta wrote:

quoted

On Tue, Feb 14, 2023 at 01:07:24AM +0000, Edgecombe, Rick P wrote:

quoted

On Mon, 2023-02-13 at 16:09 -0800, Deepak Gupta wrote:

quoted

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@rivosinc.com/ (local)

quoted

quoted

quoted

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.

quoted

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.

Yes, I understand WRSS in user mode is not safe and defeat the purpose as well.

I actually had meant why WRUSS couldn't be used in the kernel to
manufacture the token when the kernel
creates the shadow stack while parsing elf bits. But then I went
through you earlier patch series now and I've a
a little bit of context now. There is a lot of history and context
(and mess) here.

quoted

quoted

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@intel.com/ (local)

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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