| 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
| ||
|
Date: Tue, 10 Jul 2018 17:11:43 -0700
From: Dave Hansen <dave.hansen@...ux.intel.com>
To: Yu-cheng Yu <yu-cheng.yu@...el.com>, 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@...capital.net>,
Balbir Singh <bsingharora@...il.com>,
Cyrill Gorcunov <gorcunov@...il.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@...omiun.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>,
"Ravi V. Shankar" <ravi.v.shankar@...el.com>,
Vedvyas Shanbhogue <vedvyas.shanbhogue@...el.com>
Subject: Re: [RFC PATCH v2 22/27] x86/cet/ibt: User-mode indirect branch
tracking support
Is this feature *integral* to shadow stacks? Or, should it just be in a
different series?
> diff --git a/arch/x86/include/asm/cet.h b/arch/x86/include/asm/cet.h
> index d9ae3d86cdd7..71da2cccba16 100644
> --- a/arch/x86/include/asm/cet.h
> +++ b/arch/x86/include/asm/cet.h
> @@ -12,7 +12,10 @@ struct task_struct;
> struct cet_status {
> unsigned long shstk_base;
> unsigned long shstk_size;
> + unsigned long ibt_bitmap_addr;
> + unsigned long ibt_bitmap_size;
> unsigned int shstk_enabled:1;
> + unsigned int ibt_enabled:1;
> };
Is there a reason we're not using pointers here? This seems like the
kind of place that we probably want __user pointers.
> +static unsigned long ibt_mmap(unsigned long addr, unsigned long len)
> +{
> + struct mm_struct *mm = current->mm;
> + unsigned long populate;
> +
> + down_write(&mm->mmap_sem);
> + addr = do_mmap(NULL, addr, len, PROT_READ | PROT_WRITE,
> + MAP_ANONYMOUS | MAP_PRIVATE,
> + VM_DONTDUMP, 0, &populate, NULL);
> + up_write(&mm->mmap_sem);
> +
> + if (populate)
> + mm_populate(addr, populate);
> +
> + return addr;
> +}
We're going to have to start consolidating these at some point. We have
at least three of them now, maybe more.
> +int cet_setup_ibt_bitmap(void)
> +{
> + u64 r;
> + unsigned long bitmap;
> + unsigned long size;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> + return -EOPNOTSUPP;
> +
> + size = TASK_SIZE_MAX / PAGE_SIZE / BITS_PER_BYTE;
Just a note: this table is going to be gigantic on 5-level paging
systems, and userspace won't, by default use any of that extra address
space. I think it ends up being a 512GB allocation in a 128TB address
space.
Is that a problem?
On 5-level paging systems, maybe we should just stick it up in the high
part of the address space.
> + bitmap = ibt_mmap(0, size);
> +
> + if (bitmap >= TASK_SIZE_MAX)
> + return -ENOMEM;
> +
> + bitmap &= PAGE_MASK;
We're page-aligning the result of an mmap()? Why?
> + rdmsrl(MSR_IA32_U_CET, r);
> + r |= (MSR_IA32_CET_LEG_IW_EN | bitmap);
> + wrmsrl(MSR_IA32_U_CET, r);
Comments, please. What is this doing, logically? Also, why are we
OR'ing the results into this MSR? What are we trying to preserve?
> + current->thread.cet.ibt_bitmap_addr = bitmap;
> + current->thread.cet.ibt_bitmap_size = size;
> + return 0;
> +}
> +
> +void cet_disable_ibt(void)
> +{
> + u64 r;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_IBT))
> + return;
Does this need a check for being already disabled?
> + rdmsrl(MSR_IA32_U_CET, r);
> + r &= ~(MSR_IA32_CET_ENDBR_EN | MSR_IA32_CET_LEG_IW_EN |
> + MSR_IA32_CET_NO_TRACK_EN);
> + wrmsrl(MSR_IA32_U_CET, r);
> + current->thread.cet.ibt_enabled = 0;
> +}
What's the locking for current->thread.cet?
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 705467839ce8..c609c9ce5691 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -413,7 +413,8 @@ __setup("nopku", setup_disable_pku);
>
> static __always_inline void setup_cet(struct cpuinfo_x86 *c)
> {
> - if (cpu_feature_enabled(X86_FEATURE_SHSTK))
> + if (cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> + cpu_feature_enabled(X86_FEATURE_IBT))
> cr4_set_bits(X86_CR4_CET);
> }
>
> @@ -434,6 +435,23 @@ static __init int setup_disable_shstk(char *s)
> __setup("no_cet_shstk", setup_disable_shstk);
> #endif
>
> +#ifdef CONFIG_X86_INTEL_BRANCH_TRACKING_USER
> +static __init int setup_disable_ibt(char *s)
> +{
> + /* require an exact match without trailing characters */
> + if (strlen(s))
> + return 0;
> +
> + if (!boot_cpu_has(X86_FEATURE_IBT))
> + return 1;
> +
> + setup_clear_cpu_cap(X86_FEATURE_IBT);
> + pr_info("x86: 'no_cet_ibt' specified, disabling Branch Tracking\n");
> + return 1;
> +}
> +__setup("no_cet_ibt", setup_disable_ibt);
> +#endif
> /*
> * Some CPU features depend on higher CPUID levels, which may not always
> * be available due to CPUID level capping or broken virtualization
> diff --git a/arch/x86/kernel/elf.c b/arch/x86/kernel/elf.c
> index 233f6dad9c1f..42e08d3b573e 100644
> --- a/arch/x86/kernel/elf.c
> +++ b/arch/x86/kernel/elf.c
> @@ -15,6 +15,7 @@
> #include <linux/fs.h>
> #include <linux/uaccess.h>
> #include <linux/string.h>
> +#include <linux/compat.h>
>
> /*
> * The .note.gnu.property layout:
> @@ -222,7 +223,8 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
>
> struct elf64_hdr *ehdr64 = ehdr_p;
>
> - if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> + !cpu_feature_enabled(X86_FEATURE_IBT))
> return 0;
>
> if (ehdr64->e_ident[EI_CLASS] == ELFCLASS64) {
> @@ -250,6 +252,9 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
> current->thread.cet.shstk_enabled = 0;
> current->thread.cet.shstk_base = 0;
> current->thread.cet.shstk_size = 0;
> + current->thread.cet.ibt_enabled = 0;
> + current->thread.cet.ibt_bitmap_addr = 0;
> + current->thread.cet.ibt_bitmap_size = 0;
> if (cpu_feature_enabled(X86_FEATURE_SHSTK)) {
> if (shstk) {
> err = cet_setup_shstk();
> @@ -257,6 +262,15 @@ int arch_setup_features(void *ehdr_p, void *phdr_p,
> goto out;
> }
> }
> +
> + if (cpu_feature_enabled(X86_FEATURE_IBT)) {
> + if (ibt) {
> + err = cet_setup_ibt();
> + if (err < 0)
> + goto out;
> + }
> + }
You introduced 'ibt' before it was used. Please wait to introduce it
until you actually use it to make it easier to review.
Also, what's wrong with:
if (cpu_feature_enabled(X86_FEATURE_IBT) && ibt) {
...
}
?
Powered by blists - more mailing lists