Re: [PATCH RFT v8 4/9] fork: Add shadow stack support to clone3()

[Catalin Marinas <catalin.marinas@arm.com>]

On Thu, Aug 15, 2024 at 12:18:23AM +0000, Edgecombe, Rick P wrote:
> On Thu, 2024-08-08 at 09:15 +0100, Mark Brown wrote:
> > +int arch_shstk_post_fork(struct task_struct *t, struct kernel_clone_args
> > *args)
[...]
> > +       /* This should really be an atomic cmpxchg.  It is not. */
> > +       if (access_remote_vm(mm, addr, &val, sizeof(val),
> > +                            FOLL_FORCE) != sizeof(val))
> > +               goto out;
> > +
> > +       if (val != expected)
> > +               goto out;
> > +       val = 0;
> 
> 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.

> > +       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.

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.

-- 
Catalin