On Tue, Mar 14, 2023 at 12:19 AM Mike Rapoport [off-list ref] wrote:

Hi,

On Thu, Mar 09, 2023 at 10:55:11AM -0800, Deepak Gupta wrote:

quoted

On Thu, Mar 02, 2023 at 05:22:07PM +0000, Szabolcs Nagy wrote:

quoted

The 02/27/2023 14:29, Rick Edgecombe wrote:

quoted

Previously, a new PROT_SHADOW_STACK was attempted,

...

quoted

So rather than repurpose two existing syscalls (mmap, madvise) that don't
quite fit, just implement a new map_shadow_stack syscall to allow
userspace to map and setup new shadow stacks in one step. While ucontext
is the primary motivator, userspace may have other unforeseen reasons to
setup it's own shadow stacks using the WRSS instruction. Towards this
provide a flag so that stacks can be optionally setup securely for the
common case of ucontext without enabling WRSS. Or potentially have the
kernel set up the shadow stack in some new way.

...

quoted

The following example demonstrates how to create a new shadow stack with
map_shadow_stack:
void *shstk = map_shadow_stack(addr, stack_size, SHADOW_STACK_SET_TOKEN);

i think

mmap(addr, size, PROT_READ, MAP_ANON|MAP_SHADOW_STACK, -1, 0);

could do the same with less disruption to users (new syscalls
are harder to deal with than new flags). it would do the
guard page and initial token setup too (there is no flag for
it but could be squeezed in).

Discussion on this topic in v6
https://lore.kernel.org/all/20230223000340.GB945966@debug.ba.rivosinc.com/ (local)

Again I know earlier CET patches had protection flag and somehow due to pushback
on mailing list, it was adopted to go for special syscall because no one else
had shadow stack.

Seeing a response from Szabolcs, I am assuming arm4 would also want to follow
using mmap to manufacture shadow stack. For reference RFC patches for risc-v shadow stack,
use a new protection flag = PROT_SHADOWSTACK.
https://lore.kernel.org/lkml/20230213045351.3945824-1-debug@rivosinc.com/ (local)

I know earlier discussion had been that we let this go and do a re-factor later as other
arch support trickle in. But as I thought more on this and I think it may just be
messy from user mode point of view as well to have cognition of two different ways of
creating shadow stack. One would be special syscall (in current libc) and another `mmap`
(whenever future re-factor happens)

If it's not too late, it would be more wise to take `mmap`
approach rather than special `syscall` approach.

I disagree.

Having shadow stack flags for mmap() adds unnecessary complexity to the
core-mm, while having a dedicated syscall hides all the details in the
architecture specific code.

Again reiterating it would've made sense if only x86 had a shadow stack.
aarch64 announced support for guarded stack. risc-v spec is in
development to support shadow stack.

So there will be shadow stack related flow in these arches.

Another reason to use a dedicated system call allows for better
extensibility if/when we'd need to update the way shadow stack VMA is
created.

I see two valid points here
    - Shadow stack doesn't need conversion into different memory types
(which is usually the case for address ranges created by mmap)
      So there is a static page permissions on shadow stack which is
not mutable.

    - Future feature addition (if there is one needed) at the time of
shadow stack creation
      It would avoid future tax on mmap

I'll think more about this.

As for the userspace convenience, it is anyway required to add special
code for creating the shadow stack and it wouldn't matter if that code
would use mmap(NEW_FLAG) or map_shadow_stack().

Yes *strictly* from userspace convenience, it doesn't matter which option.

quoted

quoted

most of the mmap features need not be available (EINVAL) when
MAP_SHADOW_STACK is specified.

the main drawback is running out of mmap flags so extension
is limited. (but the new syscall has limitations too).

--
Sincerely yours,
Mike.