Re: [PATCH 16/33] riscv/shstk: If needed allocate a new shadow stack on clone

[Zong Li <zong.li@sifive.com>]

On Tue, Oct 8, 2024 at 1:31 PM Deepak Gupta <debug@rivosinc.com> wrote:
>
> On Tue, Oct 08, 2024 at 01:16:17PM +0800, Zong Li wrote:
> >On Tue, Oct 8, 2024 at 7:30 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >>
> >> On Mon, Oct 07, 2024 at 04:17:47PM +0800, Zong Li wrote:
> >> >On Wed, Oct 2, 2024 at 12:20 AM Deepak Gupta <debug@rivosinc.com> wrote:
> >> >>
> >> >> Userspace specifies CLONE_VM to share address space and spawn new thread.
> >> >> `clone` allow userspace to specify a new stack for new thread. However
> >> >> there is no way to specify new shadow stack base address without changing
> >> >> API. This patch allocates a new shadow stack whenever CLONE_VM is given.
> >> >>
> >> >> In case of CLONE_VFORK, parent is suspended until child finishes and thus
> >> >> can child use parent shadow stack. In case of !CLONE_VM, COW kicks in
> >> >> because entire address space is copied from parent to child.
> >> >>
> >> >> `clone3` is extensible and can provide mechanisms using which shadow stack
> >> >> as an input parameter can be provided. This is not settled yet and being
> >> >> extensively discussed on mailing list. Once that's settled, this commit
> >> >> will adapt to that.
> >> >>
> >> >> Signed-off-by: Deepak Gupta <debug@rivosinc.com>
> >> >> ---
> >> >>  arch/riscv/include/asm/usercfi.h |  25 ++++++++
> >>
> >> ... snipped...
> >>
> >> >> +
> >> >> +/*
> >> >> + * This gets called during clone/clone3/fork. And is needed to allocate a shadow stack for
> >> >> + * cases where CLONE_VM is specified and thus a different stack is specified by user. We
> >> >> + * thus need a separate shadow stack too. How does separate shadow stack is specified by
> >> >> + * user is still being debated. Once that's settled, remove this part of the comment.
> >> >> + * This function simply returns 0 if shadow stack are not supported or if separate shadow
> >> >> + * stack allocation is not needed (like in case of !CLONE_VM)
> >> >> + */
> >> >> +unsigned long shstk_alloc_thread_stack(struct task_struct *tsk,
> >> >> +                                          const struct kernel_clone_args *args)
> >> >> +{
> >> >> +       unsigned long addr, size;
> >> >> +
> >> >> +       /* If shadow stack is not supported, return 0 */
> >> >> +       if (!cpu_supports_shadow_stack())
> >> >> +               return 0;
> >> >> +
> >> >> +       /*
> >> >> +        * If shadow stack is not enabled on the new thread, skip any
> >> >> +        * switch to a new shadow stack.
> >> >> +        */
> >> >> +       if (is_shstk_enabled(tsk))
> >> >
> >> >Hi Deepak,
> >> >Should it be '!' is_shstk_enabled(tsk)?
> >>
> >> Yes it is a bug. It seems like fork without CLONE_VM or with CLONE_VFORK, it was returning
> >> 0 anyways. And in the case of CLONE_VM (used by pthread), it was not doing the right thing.
> >
> >Hi Deepak,
> >I'd like to know if I understand correctly. Could I know whether there
> >might also be a risk when the user program doesn't enable the CFI and
> >the kernel doesn't activate CFI. Because this flow will still try to
> >allocate the shadow stack and execute the ssamowap command. Thanks
>
> `shstk_alloc_thread_stack` is only called from `copy_thread` and  allocates and
> returns non-zero (positive value) for ssp only if `CLONE_VM` is specified.
> `CLONE_VM` means that address space is shared and userspace has allocated
> separate stack. This flow is ensuring that newly created thread with separate
> data stack gets a separate shadow stack as well.
>
> Retruning zero value from `shstk_alloc_thread_stack` means that, no need to
> allocate a shadow stack. If you look at `copy_thread` function, it simply sets
> the returned ssp in newly created task's task_struct (if it was non-zero).
> If returned ssp was zero, `copy_thread` doesn't do anything. Thus whatever is
> current task settings are that will be copied over to new forked/cloned task.
> If current task had shadow stack enabled, new task will also get it enabled at
> same address (to be COWed later).
>
> Any task get shadow stack enabled for first time using new prctls (see prctl
> patches).
>
> So only time `ssamoswap` will be exercised will be are
> - User issues enabling `prctl` (it'll be issued from loader)
> - fork/clone happens
>
> In both cases, it is guarded against checks of whether cpu supports it and task
> has shadow stack enabled.
>
> Let me know if you think I missed any flow.

Thanks a lot for the detail, it is very helpful for me. But sorry for
the confusion, my question is actually on the situation with this bug
(i.e., before the fix)

>
> >
> >> Most of the testing has been with busybox build (independent binaries0 driven via buildroot
> >> setup. Wondering why it wasn't caught.
> >>
> >> Anyways, will fix it. Thanks for catching it.
> >>
> >> >
> >> >> +               return 0;
> >> >> +
> >> >> +       /*