| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <d1fc2f06-b6ad-f780-72c0-cf7ec6633a30@intel.com>
Date: Fri, 9 Apr 2021 16:47:21 -0700
From: "Yu, Yu-cheng" <yu-cheng.yu@...el.com>
To: "Kirill A. Shutemov" <kirill@...temov.name>
Cc: x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-mm@...ck.org,
linux-arch@...r.kernel.org, linux-api@...r.kernel.org,
Arnd Bergmann <arnd@...db.de>,
Andy Lutomirski <luto@...nel.org>,
Balbir Singh <bsingharora@...il.com>,
Borislav Petkov <bp@...en8.de>,
Cyrill Gorcunov <gorcunov@...il.com>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Eugene Syromiatnikov <esyr@...hat.com>,
Florian Weimer <fweimer@...hat.com>,
"H.J. Lu" <hjl.tools@...il.com>, Jann Horn <jannh@...gle.com>,
Jonathan Corbet <corbet@....net>,
Kees Cook <keescook@...omium.org>,
Mike Kravetz <mike.kravetz@...cle.com>,
Nadav Amit <nadav.amit@...il.com>,
Oleg Nesterov <oleg@...hat.com>, Pavel Machek <pavel@....cz>,
Peter Zijlstra <peterz@...radead.org>,
Randy Dunlap <rdunlap@...radead.org>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
Vedvyas Shanbhogue <vedvyas.shanbhogue@...el.com>,
Dave Martin <Dave.Martin@....com>,
Weijiang Yang <weijiang.yang@...el.com>,
Pengfei Xu <pengfei.xu@...el.com>,
Haitao Huang <haitao.huang@...el.com>
Subject: Re: [PATCH v24 22/30] x86/cet/shstk: Add user-mode shadow stack
support
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@...el.com>
>> Cc: Kees Cook <keescook@...omium.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@...el.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
Powered by blists - more mailing lists