On Mon, 2023-10-23 at 19:32 +0100, Mark Brown wrote:

Right.  We're already adding the cost of the extra map_shadow_stack()
so
it doesn't seem that out of scope.  We could also allow clone3() to
be
used for allocation, potentially removing the ability to specify the
address entirely and only specifying the size.  I did consider that
option but it felt awkward in the API, though equally the whole
shadow
stack allocation thing is a bit that way.  That would avoid concerns
about placing and validating tokens entirely but gives less control
to
userspace.

There is also cost in the form of extra complexity. Not to throw FUD,
but GUP has been the source of thorny problems. And here we would be
doing it around security races. We're probably helped that shadow stack
is only private/anonymous memory, so maybe it's enough of a normal case
to not worry about it.

Still, there is some extra complexity, and I'm not sure if we really
need it. The justification seems to mostly be that it's not as flexible
as normal stacks with clone3.

I don't understand why doing size-only is awkward. Just because it
doesn't match the regular stack clone3 semantics?

This also doesn't do anything to stop anyone trying to allocate sub
page
shadow stacks if they're trying to save memory with all the lack of
overrun protection that implies, though that seems to me to be much
more
of a deliberate decision that people might make, a token would
prevent
that too unless write access to the shadow stack is enabled.

Sorry, I'm not following. Sub-page shadow stacks?

quoted

quoted

+               /*
+                * This doesn't validate that the addresses are
mapped
+                * VM_SHADOW_STACK, just that they're mapped at
all.
+                */

quoted

It just checks the range, right?

Yes, same check as for the normal stack.

What looked wrong is that the comment says that it checks if the
addresses are mapped, but the code just does access_ok(). It's a minor
thing in any case.