Messages in this thread | | | Subject | Re: [PATCH v24 22/30] x86/cet/shstk: Add user-mode shadow stack support | From | "Yu, Yu-cheng" <> | Date | Fri, 9 Apr 2021 16:47:21 -0700 |
| |
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@intel.com> >> Cc: Kees Cook <keescook@chromium.org>
[...]
>> 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@intel.com> >> + */ >> + >> +#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 = ¤t->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
| |