On Tue, Nov 14, 2023 at 11:11:58PM +0000, Edgecombe, Rick P wrote:
>On Tue, 2023-11-14 at 20:05 +0000, Mark Brown wrote:
>> +static void test_shadow_stack_supported(void)
>> +{
>> +??????? long shadow_stack;
>> +
>> +???????shadow_stack = syscall(__NR_map_shadow_stack, 0,
>> getpagesize(), 0);
>
>Hmm, x86 fails this call if user shadow stack is not supported in the
>HW or the kernel, but doesn't care if it is enabled on the thread or
>not. If shadow stack is not enabled (or not yet enabled), shadow stacks
>are allowed to be mapped. Should it fail if shadow stack is not yet
>enabled?
>
>Since shadow stack is per thread, map_shadow_stack could still be
>called on another thread that has it enabled. Basically I don't think
>blocking it will reduce the possible states the kernel has to handle.
>
>The traditional way to check if shadow stack is enabled on x86 is the
>check for a non zero return from the _get_ssp() intrinsic:
>https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/x86-control-flow-protection-intrinsics.html
>
>It seems like there will be a need for some generic method of checking
>if shadow stack is enabled. Maybe a more generic compiler
>intrinsic/builtin or glibc API (something unrelated to SSP)?
Exposing a new file under procfs would be useful?
Something like "/proc/sys/vm/user_shadow_stack_supported"
`map_shadow_stack` can return MAP_FAILED for other reasons.
I think `kselftests` are fine but I don't want people to pick up this
as test code and run with it in production :-)
So kernel providing a way to indicate whether it supports shadow stack
mappings in user mode via procfs would be useful and arch agnostic.
>
>> +???????{
>> +???????????????.name = "Shadow stack on system with shadow stack",
>> +???????????????.flags = 0,
>> +???????????????.size = 0,
>> +???????????????.expected = 0,
>> +???????????????.e2big_valid = true,
>> +???????????????.test_mode = CLONE3_ARGS_SHADOW_STACK,
>> +???????????????.filter = no_shadow_stack,
>> +???????},
>> +???????{
>> +???????????????.name = "Shadow stack on system without shadow
>> stack",
>> +???????????????.flags = 0,
>> +???????????????.size = 0,
>> +???????????????.expected = -EINVAL,
>> +???????????????.e2big_valid = true,
>> +???????????????.test_mode = CLONE3_ARGS_SHADOW_STACK,
>> +???????????????.filter = have_shadow_stack,
>> +???????},
>> ?};
>> ?
>I changed x86's map_shadow_stack to return an error when shadow stack
>was not enabled to make the detection logic in the test work. Also
>changed the clone3 Makefile to generate the shadow stack bit in the
>tests. When running the 'clone3' test with shadow stack it passed, but
>there is a failure in the non-shadow stack case:
>...
># Shadow stack not supported
>ok 20 # SKIP Shadow stack on system with shadow stack
># Running test 'Shadow stack on system without shadow stack'
># [1333] Trying clone3() with flags 0 (size 0)
># I am the parent (1333). My child's pid is 1342
># I am the child, my PID is 1342
># [1333] clone3() with flags says: 0 expected -22
># [1333] Result (0) is different than expected (-22)
>not ok 21 Shadow stack on system without shadow stack
># Totals: pass:19 fail:1 xfail:0 xpass:0 skip:1 error:0
>
>The other tests passed in both cases. I'm going to dig into the other
>parts now but can circle back if it's not obvious what's going on
>there.
On Fri, Nov 17, 2023 at 01:12:46PM -0800, Deepak Gupta wrote:
> On Tue, Nov 14, 2023 at 11:11:58PM +0000, Edgecombe, Rick P wrote:
> > It seems like there will be a need for some generic method of checking
> > if shadow stack is enabled. Maybe a more generic compiler
> > intrinsic/builtin or glibc API (something unrelated to SSP)?
> Exposing a new file under procfs would be useful?
> Something like "/proc/sys/vm/user_shadow_stack_supported"
> `map_shadow_stack` can return MAP_FAILED for other reasons.
> I think `kselftests` are fine but I don't want people to pick up this
> as test code and run with it in production :-)
> So kernel providing a way to indicate whether it supports shadow stack
> mappings in user mode via procfs would be useful and arch agnostic.
I can see that might be useful for some higher level code that wants to
tune the size and nothing else. I'd be tempted to do it through adding
a tuneable for the maximum default shadow stack size (x86 currently uses
4G) just so it's *vaguely* useful rather than just a file. I question
the utility of that but just a plain file doesn't feel quite idiomatic.
In any case it feels like it's off topic for this series and should be
done separately.