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

["Edgecombe, Rick P" <rick.p.edgecombe@intel.com>]

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)
> +{
> +       /*
> +        * SSP is aligned, so reserved bits and mode bit are a zero, just mark
> +        * the token 64-bit.
> +        */
> +       struct mm_struct *mm;
> +       unsigned long addr, ssp;
> +       u64 expected;
> +       u64 val;
> +       int ret = -EINVAL;

We should probably?
if (!features_enabled(ARCH_SHSTK_SHSTK))
	return 0;

> +
> +       ssp = args->shadow_stack + args->shadow_stack_size;
> +       addr = ssp - SS_FRAME_SIZE;
> +       expected = ssp | BIT(0);
> +
> +       mm = get_task_mm(t);
> +       if (!mm)
> +               return -EFAULT;

We could check that the VMA is shadow stack here. I'm not sure what could go
wrong though. If you point it to RW memory it could start the thread with that
as a shadow stack and just blow up at the first call. It might be nicer to fail
earlier though.

> +
> +       /* 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?

> +       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 think they are show stoppers, but the VMA check would be nice to have in
the first upstream support.

> +
> +       ret = 0;
> +
> +out:
> +       mmput(mm);
> +       return ret;
> +}
> +
> 
[snip]
> 
>  
> +static void shstk_post_fork(struct task_struct *p,
> +                           struct kernel_clone_args *args)
> +{
> +       if (!IS_ENABLED(CONFIG_ARCH_HAS_USER_SHADOW_STACK))
> +               return;
> +
> +       if (!args->shadow_stack)
> +               return;
> +
> +       if (arch_shstk_post_fork(p, args) != 0)
> +               force_sig_fault_to_task(SIGSEGV, SEGV_CPERR, NULL, p);
> +}
> +

Hmm, is this forcing the signal on the new task, which is set up on a user
provided shadow stack that failed the token check? It would handle the signal
with an arbitrary SSP then I think. We should probably fail the clone call in
the parent instead, which can be done by doing the work in copy_process(). Do
you see a problem with doing it at the end of copy_process()? I don't know if
there could be ordering constraints.