On Tue, Mar 14, 2023 at 12:19 AM Mike Rapoport <rppt@xxxxxxxxxx> wrote: > > Hi, > > On Thu, Mar 09, 2023 at 10:55:11AM -0800, Deepak Gupta wrote: > > On Thu, Mar 02, 2023 at 05:22:07PM +0000, Szabolcs Nagy wrote: > > > The 02/27/2023 14:29, Rick Edgecombe wrote: > > > > Previously, a new PROT_SHADOW_STACK was attempted, > > > ... > > > > 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. > > > ... > > > > 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@xxxxxxxxxxxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxxx/ > > > > 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. > > > > 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.
