On Fri, 2024-08-16 at 09:44 +0100, Catalin Marinas wrote:
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.
+ 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.
I don't think they are show stoppers, but the VMA check would be nice to have in the first upstream support.
Good point.