On Fri, 2024-08-16 at 09:44 +0100, Catalin Marinas wrote:

quoted

After a token is consumed normally, it doesn't set it to zero. Instead it
sets
it to a "previous-ssp token". I don't think we actually want to do that here
though because it involves the old SSP, which doesn't really apply in this
case.
I don't see any problem with zero, but was there any special thinking behind
it?

BTW, since it's the parent setting up the shadow stack in its own
address space before forking, I think at least the read can avoid
access_remote_vm() and we could do it earlier, even before the new
process is created.

Hmm. Makes sense. It's a bit racy since the parent could consume that token from
another thread, but it would be a race in any case.

quoted

quoted

+       if (access_remote_vm(mm, addr, &val, sizeof(val),
+                            FOLL_FORCE | FOLL_WRITE) != sizeof(val))
+               goto out;

The GUPs still seem a bit unfortunate for a couple reasons:
  - We could do a CMPXCHG version and are just not (I see ARM has identical
code
in gcs_consume_token()). It's not the only race like this though FWIW.
  - I *think* this is the only unprivileged FOLL_FORCE that can write to the
current process in the kernel. As is, it could be used on normal RO
mappings, at
least in a limited way. Maybe another point for the VMA check. We'd want to
check that it is normal shadow stack?
  - Lingering doubts about the wisdom of doing GUPs during task creation.

I don't like the access_remote_vm() either. In the common (practically
only) case with CLONE_VM, the mm is actually current->mm, so no need for
a GUP.

On the x86 side, we don't have a shadow stack access CMPXCHG. We will have to
GUP and do a normal CMPXCHG off of the direct map to handle it fully properly in
any case (CLONE_VM or not).

We could, in theory, consume this token in the parent before the child
mm is created. The downside is that if a parent forks multiple
processes using the same shadow stack, it will have to set the token
each time. I'd be fine with this, that's really only for the mostly
theoretical case where one doesn't use CLONE_VM and still want a
separate stack and shadow stack.

quoted

I don't think they are show stoppers, but the VMA check would be nice to
have in
the first upstream support.

Good point.