Re: [PATCH v24 22/30] x86/cet/shstk: Add user-mode shadow stack support

[Yu, Yu-cheng]

On 4/9/2021 8:57 AM, Kirill A. Shutemov wrote:
> On Thu, Apr 01, 2021 at 03:10:56PM -0700, Yu-cheng Yu wrote:
> > Introduce basic shadow stack enabling/disabling/allocation routines.
> > A task's shadow stack is allocated from memory with VM_SHADOW_STACK flag
> > and has a fixed size of min(RLIMIT_STACK, 4GB).
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>

[...]

> > diff --git a/arch/x86/kernel/shstk.c b/arch/x86/kernel/shstk.c
> > new file mode 100644
> > index 000000000000..5406fdf6df3c
> > --- /dev/null
> > +++ b/arch/x86/kernel/shstk.c
> > @@ -0,0 +1,128 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * shstk.c - Intel shadow stack support
> > + *
> > + * Copyright (c) 2021, Intel Corporation.
> > + * Yu-cheng Yu <yu-cheng.yu@xxxxxxxxx>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/mm.h>
> > +#include <linux/mman.h>
> > +#include <linux/slab.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/sched/signal.h>
> > +#include <linux/compat.h>
> > +#include <linux/sizes.h>
> > +#include <linux/user.h>
> > +#include <asm/msr.h>
> > +#include <asm/fpu/internal.h>
> > +#include <asm/fpu/xstate.h>
> > +#include <asm/fpu/types.h>
> > +#include <asm/cet.h>
> > +
> > +static void start_update_msrs(void)
> > +{
> > +	fpregs_lock();
> > +	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> > +		__fpregs_load_activate();
> > +}
> > +
> > +static void end_update_msrs(void)
> > +{
> > +	fpregs_unlock();
> > +}
> > +
> > +static unsigned long alloc_shstk(unsigned long size, int flags)
> > +{
> > +	struct mm_struct *mm = current->mm;
> > +	unsigned long addr, populate;
> > +
> > +	/* VM_SHADOW_STACK requires MAP_ANONYMOUS, MAP_PRIVATE */
> > +	flags |= MAP_ANONYMOUS | MAP_PRIVATE;
>
> Looks like all callers has flags == 0. Do I miss something.

My earlier versions use this flag.  I should have removed it.

> > +
> > +	mmap_write_lock(mm);
> > +	addr = do_mmap(NULL, 0, size, PROT_READ, flags, VM_SHADOW_STACK, 0,
> > +		       &populate, NULL);
> > +	mmap_write_unlock(mm);
> > +
> > +	if (populate)
> > +		mm_populate(addr, populate);
>
> If all callers pass down flags==0, populate will never happen.

I will fix it.

> > +
> > +	return addr;
> > +}
> > +
> > +int shstk_setup(void)
> > +{
> > +	unsigned long addr, size;
> > +	struct cet_status *cet = &current->thread.cet;
> > +
> > +	if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > +		return -EOPNOTSUPP;
> > +
> > +	size = round_up(min_t(unsigned long long, rlimit(RLIMIT_STACK), SZ_4G), PAGE_SIZE);
> > +	addr = alloc_shstk(size, 0);
> > +	if (IS_ERR_VALUE(addr))
> > +		return PTR_ERR((void *)addr);
> > +
> > +	cet->shstk_base = addr;
> > +	cet->shstk_size = size;
> > +
> > +	start_update_msrs();
> > +	wrmsrl(MSR_IA32_PL3_SSP, addr + size);
> > +	wrmsrl(MSR_IA32_U_CET, CET_SHSTK_EN);
> > +	end_update_msrs();
> > +	return 0;
> > +}
> > +
> > +void shstk_free(struct task_struct *tsk)
> > +{
> > +	struct cet_status *cet = &tsk->thread.cet;
> > +
> > +	if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> > +	    !cet->shstk_size ||
> > +	    !cet->shstk_base)
> > +		return;
> > +
> > +	if (!tsk->mm)
> > +		return;
> > +
> > +	while (1) {
> > +		int r;
> > +
> > +		r = vm_munmap(cet->shstk_base, cet->shstk_size);
> > +
> > +		/*
> > +		 * vm_munmap() returns -EINTR when mmap_lock is held by
> > +		 * something else, and that lock should not be held for a
> > +		 * long time.  Retry it for the case.
> > +		 */
>
> Hm, no. -EINTR is not about the lock being held by somebody else. The task
> got a signal and need to return to userspace.

From tracing the code itself, it looks like it cannot acquire the lock. 
 Let me dig into it.

> I have not looked at the rest of the patches yet, but why do you need a
> special free path for shadow stack? Why the normal unmap route doesn't
> work for you?

The thread's shadow stack is allocated by the kernel, so it needs to be 
freed when the thread exits.

> > +		if (r == -EINTR) {
> > +			cond_resched();
> > +			continue;
> > +		}
> > +		break;
> > +	}
> > +
> > +	cet->shstk_base = 0;
> > +	cet->shstk_size = 0;
> > +}
> > +

[...]

Thanks,
Yu-cheng