Message-ID: <ZwTQ5c+YOQFHa4YC@debug.ba.rivosinc.com>
Date: Mon, 7 Oct 2024 23:27:49 -0700
From: Deepak Gupta <debug@...osinc.com>
To: Zong Li <zong.li@...ive.com>
Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
	Borislav Petkov <bp@...en8.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>,
	Vlastimil Babka <vbabka@...e.cz>,
	Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
	Paul Walmsley <paul.walmsley@...ive.com>,
	Palmer Dabbelt <palmer@...belt.com>,
	Albert Ou <aou@...s.berkeley.edu>, Conor Dooley <conor@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Arnd Bergmann <arnd@...db.de>,
	Christian Brauner <brauner@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Oleg Nesterov <oleg@...hat.com>,
	Eric Biederman <ebiederm@...ssion.com>, Kees Cook <kees@...nel.org>,
	Jonathan Corbet <corbet@....net>, Shuah Khan <shuah@...nel.org>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-mm@...ck.org, linux-riscv@...ts.infradead.org,
	devicetree@...r.kernel.org, linux-arch@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-kselftest@...r.kernel.org,
	alistair.francis@....com, richard.henderson@...aro.org,
	jim.shu@...ive.com, andybnac@...il.com, kito.cheng@...ive.com,
	charlie@...osinc.com, atishp@...osinc.com, evan@...osinc.com,
	cleger@...osinc.com, alexghiti@...osinc.com,
	samitolvanen@...gle.com, broonie@...nel.org,
	rick.p.edgecombe@...el.com
Subject: Re: [PATCH 16/33] riscv/shstk: If needed allocate a new shadow stack
 on clone

On Tue, Oct 08, 2024 at 02:18:58PM +0800, Zong Li wrote:
>On Tue, Oct 8, 2024 at 1:31 PM Deepak Gupta <debug@...osinc.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@...osinc.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@...osinc.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@...osinc.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)


Yeah with the bug (i.e. before the fix), this function would still return 0
for `fork` or `clone` with `!CLONE_VM`. And if existing (current) thread had
shadow stack enabled, that will become shadow stack for new thread (COWed later)

The bug would surface only when `clone` is called with `CLONE_VM` and in that case
instead of allocating a new shadow stack, it would be re-using same shadow stack
for both `pthreads`.

In anycase, thanks again for noticing and bringing it up.

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

Powered by blists - more mailing lists