lkml.org 
[lkml]   [2021]   [Apr]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v24 22/30] x86/cet/shstk: Add user-mode shadow stack support
From
Date
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 = &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

\
 
 \ /
  Last update: 2021-04-10 01:48    [W:0.159 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site