* Re: [PATCH 2/5] x86/fpu/xstate: Change some names to separate XSAVES system and user states
From: Yu-cheng Yu @ 2018-06-07 15:47 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrXAx4vBUxf3VaePNm3HHLZkdTFAR9TV0T+A-jb2QL7Uag@mail.gmail.com>
On Thu, 2018-06-07 at 08:38 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > To support XSAVES system states, change some names to distinguish
> > user and system states.
> >
> > Change:
> > supervisor to system
> > copy_init_fpstate_to_fpregs() to copy_init_fpstate_user_settings_to_fpregs()
> > xfeatures_mask to xfeatures_mask_user
> > XCNTXT_MASK to SUPPORTED_XFEATURES_MASK (states supported)
>
> How about copy_init_user_fpstate_to_fpregs()? It's shorter and more
> to the point.
>
> --Andy
I will change that.
Yu-cheng
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 5/5] Documentation/x86: Add CET description
From: Yu-cheng Yu @ 2018-06-07 15:49 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrWVZfWUSOy6wRyVBfP2b2TzZuPt8bCe6q0Pa5r7onO+VA@mail.gmail.com>
On Thu, 2018-06-07 at 08:39 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Fix the subject line, please. This is more than just docs.
>
> >
> > Explain how CET works and the noshstk/noibt kernel parameters.
>
> Maybe no_cet_shstk and no_cet_ibt? noshstk sounds like gibberish and
> people might need a reminder.
I will change that.
Yu-cheng
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/9] x86/cet: Add Kconfig option for user-mode shadow stack
From: Yu-cheng Yu @ 2018-06-07 15:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrWwGCZ+Fbk+O8T6S48teHj60bQQiHQ49=SsKUOpm8VLBA@mail.gmail.com>
On Thu, 2018-06-07 at 08:47 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > Introduce Kconfig option X86_INTEL_SHADOW_STACK_USER.
> >
> > An application has shadow stack protection when all the following are
> > true:
> >
> > (1) The kernel has X86_INTEL_SHADOW_STACK_USER enabled,
> > (2) The running processor supports the shadow stack,
> > (3) The application is built with shadow stack enabled tools & libs
> > and, and at runtime, all dependent shared libs can support shadow
> > stack.
> >
> > If this kernel config option is enabled, but (2) or (3) above is not
> > true, the application runs without the shadow stack protection.
> > Existing legacy applications will continue to work without the shadow
> > stack protection.
> >
> > The user-mode shadow stack protection is only implemented for the
> > 64-bit kernel. Thirty-two bit applications are supported under the
> > compatibility mode.
> >
>
> The 64-bit only part seems entirely reasonable. So please make the
> code 64-bit only :)
Yes, I will remove changes in "arch/x86/entry/entry32.S".
We still want to support x32/ia32 in the 64-bit kernel, right?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/9] x86/mm: Introduce ptep_set_wrprotect_flush and related functions
From: Andy Lutomirski @ 2018-06-07 16:24 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143705.3531-7-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> The function ptep_set_wrprotect()/huge_ptep_set_wrprotect() is
> used by copy_page_range()/copy_hugetlb_page_range() to copy
> PTEs.
>
> On x86, when the shadow stack is enabled, only a shadow stack
> PTE has the read-only and _PAGE_DIRTY_HW combination. Upon
> making a dirty PTE read-only, we move its _PAGE_DIRTY_HW to
> _PAGE_DIRTY_SW.
>
> When ptep_set_wrprotect() moves _PAGE_DIRTY_HW to _PAGE_DIRTY_SW,
> if the PTE is writable and the mm is shared, another task could
> race to set _PAGE_DIRTY_HW again.
>
> Introduce ptep_set_wrprotect_flush(), pmdp_set_wrprotect_flush(),
> and huge_ptep_set_wrprotect_flush() to make sure this does not
> happen.
>
This patch adds flushes where they didn't previously exist.
> +static inline void ptep_set_wrprotect_flush(struct vm_area_struct *vma,
> + unsigned long addr, pte_t *ptep)
> +{
> + bool rw;
> +
> + rw = test_and_clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
> + if (IS_ENABLED(CONFIG_X86_INTEL_SHADOW_STACK_USER)) {
> + struct mm_struct *mm = vma->vm_mm;
> + pte_t pte;
> +
> + if (rw && (atomic_read(&mm->mm_users) > 1))
> + pte = ptep_clear_flush(vma, addr, ptep);
Why are you clearing the pte?
> -#define __HAVE_ARCH_PMDP_SET_WRPROTECT
> -static inline void pmdp_set_wrprotect(struct mm_struct *mm,
> - unsigned long addr, pmd_t *pmdp)
> +#define __HAVE_ARCH_HUGE_PTEP_SET_WRPROTECT_FLUSH
> +static inline void huge_ptep_set_wrprotect_flush(struct vm_area_struct *vma,
> + unsigned long addr, pte_t *ptep)
> {
> - clear_bit(_PAGE_BIT_RW, (unsigned long *)pmdp);
> + ptep_set_wrprotect_flush(vma, addr, ptep);
Maybe I'm just missing something, but you're changed the semantics of
this function significantly.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/9] x86/mm: Shadow stack page fault error checking
From: Andy Lutomirski @ 2018-06-07 16:26 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143705.3531-8-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> If a page fault is triggered by a shadow stack access (e.g.
> call/ret) or shadow stack management instructions (e.g.
> wrussq), then bit[6] of the page fault error code is set.
>
> In access_error(), we check if a shadow stack page fault
> is within a shadow stack memory area.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index 73bd8c95ac71..2b3b9170109c 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1166,6 +1166,17 @@ access_error(unsigned long error_code, struct vm_area_struct *vma)
> (error_code & X86_PF_INSTR), foreign))
> return 1;
>
> + /*
> + * Verify X86_PF_SHSTK is within a shadow stack VMA.
> + * It is always an error if there is a shadow stack
> + * fault outside a shadow stack VMA.
> + */
> + if (error_code & X86_PF_SHSTK) {
> + if (!(vma->vm_flags & VM_SHSTK))
> + return 1;
> + return 0;
> + }
> +
What, if anything, would go wrong without this change? It seems like
it might be purely an optimization. If so, can you mention that in
the comment?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/9] x86/cet: Control protection exception handler
From: Yu-cheng Yu @ 2018-06-07 16:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrVbQDyvgf5XE+a0UrTVMuhb2X=bSbp1BjGp2FAvbpSm-Q@mail.gmail.com>
On Thu, 2018-06-07 at 08:46 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
...
> > diff --git a/arch/x86/entry/entry_32.S b/arch/x86/entry/entry_32.S
> > index bef8e2b202a8..14b63ef0d7d8 100644
> > --- a/arch/x86/entry/entry_32.S
> > +++ b/arch/x86/entry/entry_32.S
> > @@ -1070,6 +1070,11 @@ ENTRY(general_protection)
> > jmp common_exception
> > END(general_protection)
> >
> > +ENTRY(control_protection)
> > + pushl $do_control_protection
> > + jmp common_exception
> > +END(control_protection)
>
> Ugh, you're seriously supporting this on 32-bit? Please test double
> fault handling very carefully -- the CET interaction with task
> switches is so gross that I didn't even bother reading the spec except
> to let the architects know that they were a but nuts to support it at
> all.
>
I will remove this.
...
> > diff --git a/arch/x86/kernel/traps.c b/arch/x86/kernel/traps.c
> > index 03f3d7695dac..4e8769a19aaf 100644
> > --- a/arch/x86/kernel/traps.c
> > +++ b/arch/x86/kernel/traps.c
>
> > +/*
> > + * When a control protection exception occurs, send a signal
> > + * to the responsible application. Currently, control
> > + * protection is only enabled for the user mode. This
> > + * exception should not come from the kernel mode.
> > + */
> > +dotraplinkage void
> > +do_control_protection(struct pt_regs *regs, long error_code)
> > +{
> > + struct task_struct *tsk;
> > +
> > + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> > + cond_local_irq_enable(regs);
> > +
> > + tsk = current;
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) &&
> > + !cpu_feature_enabled(X86_FEATURE_IBT)) {
>
> static_cpu_has(), please. But your handling here is odd -- I think
> that we should at least warn if we get #CP with CET disable.
I will fix it.
>
> > + goto exit;
> > + }
> > +
> > + if (!user_mode(regs)) {
> > + tsk->thread.error_code = error_code;
> > + tsk->thread.trap_nr = X86_TRAP_CP;
>
> I realize you copied this from elsewhere in the file, but please
> either delete these assignments to error_code and trap_nr or at least
> hoist them out of the if block.
I will fix it.
>
> > + if (notify_die(DIE_TRAP, "control protection fault", regs,
> > + error_code, X86_TRAP_CP, SIGSEGV) != NOTIFY_STOP)
>
> Does this notify_die() check serve any purpose at all? Removing all
> the old ones would be a project, but let's try not to add new callers.
OK.
>
> > + die("control protection fault", regs, error_code);
> > + return;
> > + }
> > +
> > + tsk->thread.error_code = error_code;
> > + tsk->thread.trap_nr = X86_TRAP_CP;
> > +
> > + if (show_unhandled_signals && unhandled_signal(tsk, SIGSEGV) &&
> > + printk_ratelimit()) {
> > + unsigned int max_idx, err_idx;
> > +
> > + max_idx = ARRAY_SIZE(control_protection_err) - 1;
> > + err_idx = min((unsigned int)error_code - 1, max_idx);
>
> What if error_code == 0? Is that also invalid?
The error code is between 1 and 5 inclusive. I thought if it is 0, then
err_idx would become max_idx here. I can change it to:
if (error_code == 0)
error_code = max_idx;
Or, add some comments for this case.
>
> > + pr_info("%s[%d] control protection ip:%lx sp:%lx error:%lx(%s)",
> > + tsk->comm, task_pid_nr(tsk),
> > + regs->ip, regs->sp, error_code,
> > + control_protection_err[err_idx]);
> > + print_vma_addr(" in ", regs->ip);
> > + pr_cont("\n");
> > + }
> > +
> > +exit:
> > + force_sig_info(SIGSEGV, SEND_SIG_PRIV, tsk);
>
> This is definitely wrong for the feature-disabled, !user_mode case.
>
I will fix it.
> Also, are you planning on enabling CET for kernel code too?
Yes, kernel protection will be enabled later.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/9] x86/cet: Add Kconfig option for user-mode shadow stack
From: Andy Lutomirski @ 2018-06-07 16:28 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: Andrew Lutomirski, LKML, linux-doc, Linux-MM, linux-arch, X86 ML,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H. J. Lu,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <1528387137.4636.6.camel@2b52.sc.intel.com>
On Thu, Jun 7, 2018 at 9:02 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> On Thu, 2018-06-07 at 08:47 -0700, Andy Lutomirski wrote:
> > On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > >
> > > Introduce Kconfig option X86_INTEL_SHADOW_STACK_USER.
> > >
> > > An application has shadow stack protection when all the following are
> > > true:
> > >
> > > (1) The kernel has X86_INTEL_SHADOW_STACK_USER enabled,
> > > (2) The running processor supports the shadow stack,
> > > (3) The application is built with shadow stack enabled tools & libs
> > > and, and at runtime, all dependent shared libs can support shadow
> > > stack.
> > >
> > > If this kernel config option is enabled, but (2) or (3) above is not
> > > true, the application runs without the shadow stack protection.
> > > Existing legacy applications will continue to work without the shadow
> > > stack protection.
> > >
> > > The user-mode shadow stack protection is only implemented for the
> > > 64-bit kernel. Thirty-two bit applications are supported under the
> > > compatibility mode.
> > >
> >
> > The 64-bit only part seems entirely reasonable. So please make the
> > code 64-bit only :)
>
> Yes, I will remove changes in "arch/x86/entry/entry32.S".
> We still want to support x32/ia32 in the 64-bit kernel, right?
>
Yes, I think. But that's not in entry_32.S
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 01/10] x86/cet: User-mode shadow stack support
From: Andy Lutomirski @ 2018-06-07 16:37 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143807.3611-2-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> This patch adds basic shadow stack enabling/disabling routines.
> A task's shadow stack is allocated from memory with VM_SHSTK
> flag set and read-only protection. The shadow stack is
> allocated to a fixed size and that can be changed by the system
> admin.
How do threads work? Can a user program mremap() its shadow stack to
make it bigger?
Also, did you add all the needed checks to make get_user_pages(),
access_process_vm(), etc fail when called on the shadow stack? (Or at
least fail if they're requesting write access and the FORCE bit isn't
set.)
> +#define SHSTK_SIZE (0x8000 * (test_thread_flag(TIF_IA32) ? 4 : 8))
Please don't add more mode-dependent #defines. Also, please try to
avoid adding any new code that looks at TIF_IA32 or similar. Uses of
that bit are generally bugs, and the bit itself should get removed
some day. If you need to make a guess, use in_compat_syscall() or
similar if appropriate.
> +
> +static inline int cet_set_shstk_ptr(unsigned long addr)
> +{
> + u64 r;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return -1;
> +
> + if ((addr >= TASK_SIZE) || (!IS_ALIGNED(addr, 4)))
> + return -1;'
TASK_SIZE_MAX, please. TASK_SIZE is weird and is usually the wrong
thing to use.
> +static unsigned long shstk_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,
> + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
> + 0, &populate, NULL);
> + up_write(&mm->mmap_sem);
> +
> + if (populate)
> + mm_populate(addr, populate);
Please don't populate if do_mmap() failed.
> +int cet_setup_shstk(void)
> +{
> + unsigned long addr, size;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return -EOPNOTSUPP;
> +
> + size = SHSTK_SIZE;
> + addr = shstk_mmap(0, size);
> +
> + if (addr >= TASK_SIZE)
> + return -ENOMEM;
Please check the actual value that do_mmap() would return on error.
(IS_ERR, 0, MAP_FAILED -- I don't remember.)
> +
> + cet_set_shstk_ptr(addr + size - sizeof(void *));
> + current->thread.cet.shstk_base = addr;
> + current->thread.cet.shstk_size = size;
> + current->thread.cet.shstk_enabled = 1;
> + return 0;
> +}
> +
> +void cet_disable_shstk(void)
> +{
> + u64 r;
> +
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> + return;
> +
> + rdmsrl(MSR_IA32_U_CET, r);
> + r &= ~(MSR_IA32_CET_SHSTK_EN);
> + wrmsrl(MSR_IA32_U_CET, r);
> + wrmsrl(MSR_IA32_PL3_SSP, 0);
> + current->thread.cet.shstk_enabled = 0;
> +}
> +
> +void cet_disable_free_shstk(struct task_struct *tsk)
> +{
> + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> + !tsk->thread.cet.shstk_enabled)
> + return;
> +
> + if (tsk == current)
> + cet_disable_shstk();
if tsk != current, then this will malfunction, right? What is it
intended to do?
> +
> + /*
> + * Free only when tsk is current or shares mm
> + * with current but has its own shstk.
> + */
> + if (tsk->mm && (tsk->mm == current->mm) &&
> + (tsk->thread.cet.shstk_base)) {
> + vm_munmap(tsk->thread.cet.shstk_base,
> + tsk->thread.cet.shstk_size);
> + tsk->thread.cet.shstk_base = 0;
> + tsk->thread.cet.shstk_size = 0;
> + }
I'm having trouble imagining why the kernel would ever want to
automatically free the shadow stack vma. What is this for?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 02/10] x86/cet: Introduce WRUSS instruction
From: Andy Lutomirski @ 2018-06-07 16:40 UTC (permalink / raw)
To: Yu-cheng Yu, Peter Zijlstra
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143807.3611-3-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> WRUSS is a new kernel-mode instruction but writes directly
> to user shadow stack memory. This is used to construct
> a return address on the shadow stack for the signal
> handler.
>
> This instruction can fault if the user shadow stack is
> invalid shadow stack memory. In that case, the kernel does
> fixup.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/special_insns.h | 44 +++++++++++++++++++++++++++
> arch/x86/lib/x86-opcode-map.txt | 2 +-
> arch/x86/mm/fault.c | 13 +++++++-
> tools/objtool/arch/x86/lib/x86-opcode-map.txt | 2 +-
> 4 files changed, 58 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> index 317fc59b512c..8ce532fcc171 100644
> --- a/arch/x86/include/asm/special_insns.h
> +++ b/arch/x86/include/asm/special_insns.h
> @@ -237,6 +237,50 @@ static inline void clwb(volatile void *__p)
> : [pax] "a" (p));
> }
>
> +#ifdef CONFIG_X86_INTEL_CET
> +
> +#if defined(CONFIG_IA32_EMULATION) || defined(CONFIG_X86_X32)
> +static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
> +{
> + int err;
> +
Please add a comment indicating what exact opcode this is.
Peterz, isn't there some fancy better way we're supposed to handle the
error return these days?
> + asm volatile("1:.byte 0x66, 0x0f, 0x38, 0xf5, 0x37\n"
> + "xor %[err],%[err]\n"
> + "2:\n"
> + ".section .fixup,\"ax\"\n"
> + "3: mov $-1,%[err]; jmp 2b\n"
> + ".previous\n"
> + _ASM_EXTABLE(1b, 3b)
> + : [err] "=a" (err)
> + : [val] "S" (val), [addr] "D" (addr)
> + : "memory");
> + return err;
> +}
> +#else
> +static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
> +{
> + return 0;
BUG()? Or just omit the ifdef? It seems unhelpful to have a stub
function that does nothing.
> +}
> +#endif
> +
> +static inline int write_user_shstk_64(unsigned long addr, unsigned long val)
> +{
> + int err;
> +
Comment here too, please.
> + asm volatile("1:.byte 0x66, 0x48, 0x0f, 0x38, 0xf5, 0x37\n"
> + "xor %[err],%[err]\n"
> + "2:\n"
> + ".section .fixup,\"ax\"\n"
> + "3: mov $-1,%[err]; jmp 2b\n"
> + ".previous\n"
> + _ASM_EXTABLE(1b, 3b)
> + : [err] "=a" (err)
> + : [val] "S" (val), [addr] "D" (addr)
> + : "memory");
> + return err;
> +}
> +#endif /* CONFIG_X86_INTEL_CET */
> +
> #define nop() asm volatile ("nop")
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 1/7] x86/cet: Add Kconfig option for user-mode Indirect Branch Tracking
From: Randy Dunlap @ 2018-06-07 16:43 UTC (permalink / raw)
To: Yu-cheng Yu, linux-kernel, linux-doc, linux-mm, linux-arch, x86,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H.J. Lu,
Vedvyas Shanbhogue, Ravi V. Shankar, Dave Hansen, Andy Lutomirski,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, Mike Kravetz
In-Reply-To: <20180607143855.3681-2-yu-cheng.yu@intel.com>
On 06/07/2018 07:38 AM, Yu-cheng Yu wrote:
> The user-mode indirect branch tracking support is done mostly by
> GCC to insert ENDBR64/ENDBR32 instructions at branch targets.
> The kernel provides CPUID enumeration, feature MSR setup and
> the allocation of legacy bitmap.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/Kconfig | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 24339a5299da..27bfbd137fbe 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1953,6 +1953,18 @@ config X86_INTEL_SHADOW_STACK_USER
>
> If unsure, say y.
>
> +config X86_INTEL_BRANCH_TRACKING_USER
> + prompt "Intel Indirect Branch Tracking for user-mode"
> + def_bool n
> + depends on CPU_SUP_INTEL && X86_64
> + select X86_INTEL_CET
> + select ARCH_HAS_PROGRAM_PROPERTIES
> + ---help---
> + Indirect Branch Tracking provides hardware protection against
> + oriented programing attacks.
programming
and please just move the return/jmp parts to the next line also:
protection against
return-/jmp-oriented programming attacks.
> +
> + If unsure, say y
> +
> config EFI
> bool "EFI runtime service support"
> depends on ACPI
>
--
~Randy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/9] x86/mm: Shadow stack page fault error checking
From: Yu-cheng Yu @ 2018-06-07 16:46 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrXA--XrVNvPM4-Cv6-E6OFd=TZ5Gw_MWePt7MtqCBBqRg@mail.gmail.com>
On Thu, 2018-06-07 at 09:26 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:40 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > If a page fault is triggered by a shadow stack access (e.g.
> > call/ret) or shadow stack management instructions (e.g.
> > wrussq), then bit[6] of the page fault error code is set.
> >
> > In access_error(), we check if a shadow stack page fault
> > is within a shadow stack memory area.
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
>
> > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > index 73bd8c95ac71..2b3b9170109c 100644
> > --- a/arch/x86/mm/fault.c
> > +++ b/arch/x86/mm/fault.c
> > @@ -1166,6 +1166,17 @@ access_error(unsigned long error_code, struct vm_area_struct *vma)
> > (error_code & X86_PF_INSTR), foreign))
> > return 1;
> >
> > + /*
> > + * Verify X86_PF_SHSTK is within a shadow stack VMA.
> > + * It is always an error if there is a shadow stack
> > + * fault outside a shadow stack VMA.
> > + */
> > + if (error_code & X86_PF_SHSTK) {
> > + if (!(vma->vm_flags & VM_SHSTK))
> > + return 1;
> > + return 0;
> > + }
> > +
>
> What, if anything, would go wrong without this change? It seems like
> it might be purely an optimization. If so, can you mention that in
> the comment?
Without this check, the page fault code could overlook the fact that the
application is trying to use non shadow stack area for shadow stack.
I will add this to the comments.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 02/10] x86/cet: Introduce WRUSS instruction
From: Yu-cheng Yu @ 2018-06-07 16:51 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Peter Zijlstra, LKML, linux-doc, Linux-MM, linux-arch, X86 ML,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H. J. Lu,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrU45Cuzvfz3c1+-+7=9KS2N33Bpp1JqBtaGxhPo8U+Fqg@mail.gmail.com>
On Thu, 2018-06-07 at 09:40 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > WRUSS is a new kernel-mode instruction but writes directly
> > to user shadow stack memory. This is used to construct
> > a return address on the shadow stack for the signal
> > handler.
> >
> > This instruction can fault if the user shadow stack is
> > invalid shadow stack memory. In that case, the kernel does
> > fixup.
> >
> > Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> > ---
> > arch/x86/include/asm/special_insns.h | 44 +++++++++++++++++++++++++++
> > arch/x86/lib/x86-opcode-map.txt | 2 +-
> > arch/x86/mm/fault.c | 13 +++++++-
> > tools/objtool/arch/x86/lib/x86-opcode-map.txt | 2 +-
> > 4 files changed, 58 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
> > index 317fc59b512c..8ce532fcc171 100644
> > --- a/arch/x86/include/asm/special_insns.h
> > +++ b/arch/x86/include/asm/special_insns.h
> > @@ -237,6 +237,50 @@ static inline void clwb(volatile void *__p)
> > : [pax] "a" (p));
> > }
> >
> > +#ifdef CONFIG_X86_INTEL_CET
> > +
> > +#if defined(CONFIG_IA32_EMULATION) || defined(CONFIG_X86_X32)
> > +static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
> > +{
> > + int err;
> > +
>
> Please add a comment indicating what exact opcode this is.
I will fix it.
>
> Peterz, isn't there some fancy better way we're supposed to handle the
> error return these days?
>
> > + asm volatile("1:.byte 0x66, 0x0f, 0x38, 0xf5, 0x37\n"
> > + "xor %[err],%[err]\n"
> > + "2:\n"
> > + ".section .fixup,\"ax\"\n"
> > + "3: mov $-1,%[err]; jmp 2b\n"
> > + ".previous\n"
> > + _ASM_EXTABLE(1b, 3b)
> > + : [err] "=a" (err)
> > + : [val] "S" (val), [addr] "D" (addr)
> > + : "memory");
> > + return err;
> > +}
> > +#else
> > +static inline int write_user_shstk_32(unsigned long addr, unsigned int val)
> > +{
> > + return 0;
>
> BUG()? Or just omit the ifdef? It seems unhelpful to have a stub
> function that does nothing.
I will fix it.
>
> > +}
> > +#endif
> > +
> > +static inline int write_user_shstk_64(unsigned long addr, unsigned long val)
> > +{
> > + int err;
> > +
>
> Comment here too, please.
OK.
>
> > + asm volatile("1:.byte 0x66, 0x48, 0x0f, 0x38, 0xf5, 0x37\n"
> > + "xor %[err],%[err]\n"
> > + "2:\n"
> > + ".section .fixup,\"ax\"\n"
> > + "3: mov $-1,%[err]; jmp 2b\n"
> > + ".previous\n"
> > + _ASM_EXTABLE(1b, 3b)
> > + : [err] "=a" (err)
> > + : [val] "S" (val), [addr] "D" (addr)
> > + : "memory");
> > + return err;
> > +}
> > +#endif /* CONFIG_X86_INTEL_CET */
> > +
> > #define nop() asm volatile ("nop")
> >
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/9] x86/mm: Shadow stack page fault error checking
From: Dave Hansen @ 2018-06-07 16:56 UTC (permalink / raw)
To: Andy Lutomirski, Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Jonathan Corbet, Oleg Nesterov, Arnd Bergmann,
mike.kravetz
In-Reply-To: <CALCETrXA--XrVNvPM4-Cv6-E6OFd=TZ5Gw_MWePt7MtqCBBqRg@mail.gmail.com>
On 06/07/2018 09:26 AM, Andy Lutomirski wrote:
>>
>> + /*
>> + * Verify X86_PF_SHSTK is within a shadow stack VMA.
>> + * It is always an error if there is a shadow stack
>> + * fault outside a shadow stack VMA.
>> + */
>> + if (error_code & X86_PF_SHSTK) {
>> + if (!(vma->vm_flags & VM_SHSTK))
>> + return 1;
>> + return 0;
>> + }
>> +
> What, if anything, would go wrong without this change? It seems like
> it might be purely an optimization. If so, can you mention that in
> the comment?
This is a fine exercise. I'm curious what it does, too.
But, I really like it being explicit in the end. If we depend on
implicit behavior, I really worry that someone breaks it accidentally.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 01/10] x86/cet: User-mode shadow stack support
From: Yu-cheng Yu @ 2018-06-07 17:46 UTC (permalink / raw)
To: Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrX4ALKbphJiZs4MXWtRFvQYD905bNAMTogbOeLh0Pp6xw@mail.gmail.com>
On Thu, 2018-06-07 at 09:37 -0700, Andy Lutomirski wrote:
> On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> >
> > This patch adds basic shadow stack enabling/disabling routines.
> > A task's shadow stack is allocated from memory with VM_SHSTK
> > flag set and read-only protection. The shadow stack is
> > allocated to a fixed size and that can be changed by the system
> > admin.
>
> How do threads work? Can a user program mremap() its shadow stack to
> make it bigger?
A pthread's shadow stack is allocated/freed by the kernel. This patch
has the supporting routines that handle both non-pthread and pthread.
In [PATCH 04/10] "Handle thread shadow stack", we allocate pthread
shadow stack in copy_thread_tls(), and free it in deactivate_mm().
If clone of a pthread fails, shadow stack is freed in
cet_disable_free_shstk() below (I will add more comments):
If (Current thread existing)
Disable and free shadow stack
If (Clone of a pthread fails)
Free the pthread shadow stack
We block mremap, mprotect, madvise, and munmap on a vma that has
VM_SHSTK (in separate patches).
> Also, did you add all the needed checks to make get_user_pages(),
> access_process_vm(), etc fail when called on the shadow stack? (Or at
> least fail if they're requesting write access and the FORCE bit isn't
> set.)
Currently if FORCE bit is set, these functions can write to shadow
stack, otherwise write access will fail. I will test it.
> > +#define SHSTK_SIZE (0x8000 * (test_thread_flag(TIF_IA32) ? 4 : 8))
>
> Please don't add more mode-dependent #defines. Also, please try to
> avoid adding any new code that looks at TIF_IA32 or similar. Uses of
> that bit are generally bugs, and the bit itself should get removed
> some day. If you need to make a guess, use in_compat_syscall() or
> similar if appropriate.
OK.
>
> > +
> > +static inline int cet_set_shstk_ptr(unsigned long addr)
> > +{
> > + u64 r;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + return -1;
> > +
> > + if ((addr >= TASK_SIZE) || (!IS_ALIGNED(addr, 4)))
> > + return -1;'
>
> TASK_SIZE_MAX, please. TASK_SIZE is weird and is usually the wrong
> thing to use.
OK.
>
> > +static unsigned long shstk_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,
> > + MAP_ANONYMOUS | MAP_PRIVATE, VM_SHSTK,
> > + 0, &populate, NULL);
> > + up_write(&mm->mmap_sem);
> > +
> > + if (populate)
> > + mm_populate(addr, populate);
>
> Please don't populate if do_mmap() failed.
I will fix it.
>
> > +int cet_setup_shstk(void)
> > +{
> > + unsigned long addr, size;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + return -EOPNOTSUPP;
> > +
> > + size = SHSTK_SIZE;
> > + addr = shstk_mmap(0, size);
> > +
> > + if (addr >= TASK_SIZE)
> > + return -ENOMEM;
>
> Please check the actual value that do_mmap() would return on error.
> (IS_ERR, 0, MAP_FAILED -- I don't remember.)
OK.
>
> > +
> > + cet_set_shstk_ptr(addr + size - sizeof(void *));
> > + current->thread.cet.shstk_base = addr;
> > + current->thread.cet.shstk_size = size;
> > + current->thread.cet.shstk_enabled = 1;
> > + return 0;
> > +}
> > +
> > +void cet_disable_shstk(void)
> > +{
> > + u64 r;
> > +
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK))
> > + return;
> > +
> > + rdmsrl(MSR_IA32_U_CET, r);
> > + r &= ~(MSR_IA32_CET_SHSTK_EN);
> > + wrmsrl(MSR_IA32_U_CET, r);
> > + wrmsrl(MSR_IA32_PL3_SSP, 0);
> > + current->thread.cet.shstk_enabled = 0;
> > +}
> > +
> > +void cet_disable_free_shstk(struct task_struct *tsk)
> > +{
> > + if (!cpu_feature_enabled(X86_FEATURE_SHSTK) ||
> > + !tsk->thread.cet.shstk_enabled)
> > + return;
> > +
> > + if (tsk == current)
> > + cet_disable_shstk();
>
> if tsk != current, then this will malfunction, right? What is it
> intended to do?
We get here when clone fails. In this condition, we don't disable the
calling task's shadow stack. I will add comments.
>
> > +
> > + /*
> > + * Free only when tsk is current or shares mm
> > + * with current but has its own shstk.
> > + */
> > + if (tsk->mm && (tsk->mm == current->mm) &&
> > + (tsk->thread.cet.shstk_base)) {
> > + vm_munmap(tsk->thread.cet.shstk_base,
> > + tsk->thread.cet.shstk_size);
> > + tsk->thread.cet.shstk_base = 0;
> > + tsk->thread.cet.shstk_size = 0;
> > + }
>
> I'm having trouble imagining why the kernel would ever want to
> automatically free the shadow stack vma. What is this for?
This is for pthreads. When a pthread exits, its shadow stack needs to
be freed.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 01/10] x86/cet: User-mode shadow stack support
From: Dave Hansen @ 2018-06-07 17:55 UTC (permalink / raw)
To: Yu-cheng Yu, Andy Lutomirski
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Jonathan Corbet, Oleg Nesterov, Arnd Bergmann,
mike.kravetz
In-Reply-To: <1528393611.4636.70.camel@2b52.sc.intel.com>
On 06/07/2018 10:46 AM, Yu-cheng Yu wrote:
>> Also, did you add all the needed checks to make get_user_pages(),
>> access_process_vm(), etc fail when called on the shadow stack? (Or at
>> least fail if they're requesting write access and the FORCE bit isn't
>> set.)
> Currently if FORCE bit is set, these functions can write to shadow
> stack, otherwise write access will fail. I will test it.
Is this a part of your selftests/ for this feature?
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 04/10] x86/cet: Handle thread shadow stack
From: Andy Lutomirski @ 2018-06-07 18:21 UTC (permalink / raw)
To: Yu-cheng Yu, Florian Weimer
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143807.3611-5-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> When fork() specifies CLONE_VM but not CLONE_VFORK, the child
> needs a separate program stack and a separate shadow stack.
> This patch handles allocation and freeing of the thread shadow
> stack.
Aha -- you're trying to make this automatic. I'm not convinced this
is a good idea. The Linux kernel has a long and storied history of
enabling new hardware features in ways that are almost entirely
useless for userspace.
Florian, do you have any thoughts on how the user/kernel interaction
for the shadow stack should work? My intuition would be that all
shadow stack management should be entirely controlled by userspace --
newly cloned threads (with CLONE_VM) should have no shadow stack
initially, and newly started processes should have no shadow stack
until they ask for one. If it would be needed for optimization, there
could some indication in an ELF binary that it is requesting an
initial shadow stack.
But maybe some kind of automation like this patch does is actually reasonable.
--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/9] x86/mm: Introduce ptep_set_wrprotect_flush and related functions
From: Dave Hansen @ 2018-06-07 18:21 UTC (permalink / raw)
To: Andy Lutomirski, Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Jonathan Corbet, Oleg Nesterov, Arnd Bergmann,
mike.kravetz
In-Reply-To: <CALCETrVa8MtxP9iqYkZLnetaQiN4UaWb=jGz1+rLsCuETHKydg@mail.gmail.com>
On 06/07/2018 09:24 AM, Andy Lutomirski wrote:
>> +static inline void ptep_set_wrprotect_flush(struct vm_area_struct *vma,
>> + unsigned long addr, pte_t *ptep)
>> +{
>> + bool rw;
>> +
>> + rw = test_and_clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
>> + if (IS_ENABLED(CONFIG_X86_INTEL_SHADOW_STACK_USER)) {
>> + struct mm_struct *mm = vma->vm_mm;
>> + pte_t pte;
>> +
>> + if (rw && (atomic_read(&mm->mm_users) > 1))
>> + pte = ptep_clear_flush(vma, addr, ptep);
> Why are you clearing the pte?
I think I insisted on this being in there.
First of all, we need to flush the TLB eventually because we need the
shadowstack PTE permissions to be in effect.
But, generally, we can't clear a dirty bit in a "live" PTE without
flushing. The processor can keep writing until we flush, and even keep
setting it whenever _it_ allows a write, which it can do based on stale
TLB contents. Practically, I think a walk to set the dirty bit is
mostly the same as a TLB miss, but that's certainly not guaranteed forever.
That's even ignoring all the fun errata we have.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 01/10] x86/cet: User-mode shadow stack support
From: Andy Lutomirski @ 2018-06-07 18:23 UTC (permalink / raw)
To: Yu-cheng Yu, Florian Weimer
Cc: Andrew Lutomirski, LKML, linux-doc, Linux-MM, linux-arch, X86 ML,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H. J. Lu,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <1528393611.4636.70.camel@2b52.sc.intel.com>
On Thu, Jun 7, 2018 at 10:50 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> On Thu, 2018-06-07 at 09:37 -0700, Andy Lutomirski wrote:
> > On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
> > >
> > > This patch adds basic shadow stack enabling/disabling routines.
> > > A task's shadow stack is allocated from memory with VM_SHSTK
> > > flag set and read-only protection. The shadow stack is
> > > allocated to a fixed size and that can be changed by the system
> > > admin.
> >
> > How do threads work? Can a user program mremap() its shadow stack to
> > make it bigger?
>
> A pthread's shadow stack is allocated/freed by the kernel. This patch
> has the supporting routines that handle both non-pthread and pthread.
>
> In [PATCH 04/10] "Handle thread shadow stack", we allocate pthread
> shadow stack in copy_thread_tls(), and free it in deactivate_mm().
>
> If clone of a pthread fails, shadow stack is freed in
> cet_disable_free_shstk() below (I will add more comments):
>
> If (Current thread existing)
> Disable and free shadow stack
>
> If (Clone of a pthread fails)
> Free the pthread shadow stack
>
> We block mremap, mprotect, madvise, and munmap on a vma that has
> VM_SHSTK (in separate patches).
Why? mremap() seems like a sensible way to enlarge a shadow stack.
munmap() seems like a good way to get rid of one, and mmap() seems
like a nice way to create a new shadow stack if one were needed (for
green threads or similar).
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/9] x86/mm: Introduce ptep_set_wrprotect_flush and related functions
From: Andy Lutomirski @ 2018-06-07 18:24 UTC (permalink / raw)
To: Dave Hansen
Cc: Yu-cheng Yu, LKML, linux-doc, Linux-MM, linux-arch, X86 ML,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H. J. Lu,
Shanbhogue, Vedvyas, Ravi V. Shankar, Jonathan Corbet,
Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <d9939c23-bb8b-1fbc-ac65-8bf1d7cbf650@linux.intel.com>
On Thu, Jun 7, 2018 at 11:23 AM Dave Hansen <dave.hansen@linux.intel.com> wrote:
>
> On 06/07/2018 09:24 AM, Andy Lutomirski wrote:
> >> +static inline void ptep_set_wrprotect_flush(struct vm_area_struct *vma,
> >> + unsigned long addr, pte_t *ptep)
> >> +{
> >> + bool rw;
> >> +
> >> + rw = test_and_clear_bit(_PAGE_BIT_RW, (unsigned long *)&ptep->pte);
> >> + if (IS_ENABLED(CONFIG_X86_INTEL_SHADOW_STACK_USER)) {
> >> + struct mm_struct *mm = vma->vm_mm;
> >> + pte_t pte;
> >> +
> >> + if (rw && (atomic_read(&mm->mm_users) > 1))
> >> + pte = ptep_clear_flush(vma, addr, ptep);
> > Why are you clearing the pte?
>
> I think I insisted on this being in there.
>
> First of all, we need to flush the TLB eventually because we need the
> shadowstack PTE permissions to be in effect.
>
> But, generally, we can't clear a dirty bit in a "live" PTE without
> flushing. The processor can keep writing until we flush, and even keep
> setting it whenever _it_ allows a write, which it can do based on stale
> TLB contents. Practically, I think a walk to set the dirty bit is
> mostly the same as a TLB miss, but that's certainly not guaranteed forever.
>
> That's even ignoring all the fun errata we have.
Maybe make it a separate patch, then? It seems like this patch is
doing two almost entirely separate things: adding some flushes and
adding the magic hwdirty -> swdirty transition. As it stands, it's
quite difficult to read the patch and figure out what it does.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 03/10] x86/cet: Signal handling for shadow stack
From: Andy Lutomirski @ 2018-06-07 18:30 UTC (permalink / raw)
To: Yu-cheng Yu, Florian Weimer, Dmitry Safonov, Cyrill Gorcunov
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143807.3611-4-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Set and restore shadow stack pointer for signals.
How does this interact with siglongjmp()?
This patch makes me extremely nervous due to the possibility of ABI
issues and CRIU breakage.
> diff --git a/arch/x86/include/uapi/asm/sigcontext.h b/arch/x86/include/uapi/asm/sigcontext.h
> index 844d60eb1882..6c8997a0156a 100644
> --- a/arch/x86/include/uapi/asm/sigcontext.h
> +++ b/arch/x86/include/uapi/asm/sigcontext.h
> @@ -230,6 +230,7 @@ struct sigcontext_32 {
> __u32 fpstate; /* Zero when no FPU/extended context */
> __u32 oldmask;
> __u32 cr2;
> + __u32 ssp;
> };
>
> /*
> @@ -262,6 +263,7 @@ struct sigcontext_64 {
> __u64 trapno;
> __u64 oldmask;
> __u64 cr2;
> + __u64 ssp;
>
> /*
> * fpstate is really (struct _fpstate *) or (struct _xstate *)
> @@ -320,6 +322,7 @@ struct sigcontext {
> struct _fpstate __user *fpstate;
> __u32 oldmask;
> __u32 cr2;
> + __u32 ssp;
Is it actually okay to modify these structures like this? They're
part of the user ABI, and I don't know whether any user code relies on
the size being constant.
> +int cet_push_shstk(int ia32, unsigned long ssp, unsigned long val)
> +{
> + if (val >= TASK_SIZE)
> + return -EINVAL;
TASK_SIZE_MAX. But I'm a bit unsure why you need this check at all.
> +int cet_restore_signal(unsigned long ssp)
> +{
> + if (!current->thread.cet.shstk_enabled)
> + return 0;
> + return cet_set_shstk_ptr(ssp);
> +}
This will blow up if the shadow stack enabled state changes in a
signal handler. Maybe we don't care.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH RESEND] Documentation: filesystems: update filesystem locking documentation
From: Al Viro @ 2018-06-07 18:31 UTC (permalink / raw)
To: Sean Anderson
Cc: corbet, linux-doc, linux-fsdevel, Matthew Wilcox, Jeff Layton
In-Reply-To: <86caf9d3-a520-bb4c-b174-954966c844e4@gmail.com>
On Tue, Jun 05, 2018 at 03:46:37PM -0400, Sean Anderson wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> > Your mission, should you choose to accept it, shall be to locate
> > the old sig regarding the usual reaction to use of
> > Quoted-Printable...
> >
> > IOW, fix your mail setup. Applied, but.
>
> Not sure what you mean by "Quoted-Printable"... Is PGP/MIME the
> incorrect way to send messages? I've switched to the "traditional"
> option for this one, and I've enabled "mail.strictly_mime" in my
> settings. Let me know if this fixes things for you.
Useful test: if git am barfs on your mail (which it does for the
first posting in the thread), you are going to annoy people...
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 7/7] x86/cet: Add PTRACE interface for CET
From: Andy Lutomirski @ 2018-06-07 18:32 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143855.3681-8-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:42 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Add PTRACE interface for CET MSRs.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/asm/fpu/regset.h | 7 ++++---
> arch/x86/kernel/fpu/regset.c | 41 +++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/ptrace.c | 16 +++++++++++++++
> include/uapi/linux/elf.h | 1 +
> 4 files changed, 62 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/regset.h b/arch/x86/include/asm/fpu/regset.h
> index d5bdffb9d27f..edad0d889084 100644
> --- a/arch/x86/include/asm/fpu/regset.h
> +++ b/arch/x86/include/asm/fpu/regset.h
> @@ -7,11 +7,12 @@
>
> #include <linux/regset.h>
>
> -extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active;
> +extern user_regset_active_fn regset_fpregs_active, regset_xregset_fpregs_active,
> + cetregs_active;
> extern user_regset_get_fn fpregs_get, xfpregs_get, fpregs_soft_get,
> - xstateregs_get;
> + xstateregs_get, cetregs_get;
> extern user_regset_set_fn fpregs_set, xfpregs_set, fpregs_soft_set,
> - xstateregs_set;
> + xstateregs_set, cetregs_set;
>
> /*
> * xstateregs_active == regset_fpregs_active. Please refer to the comment
> diff --git a/arch/x86/kernel/fpu/regset.c b/arch/x86/kernel/fpu/regset.c
> index bc02f5144b95..7008eb084d36 100644
> --- a/arch/x86/kernel/fpu/regset.c
> +++ b/arch/x86/kernel/fpu/regset.c
> @@ -160,6 +160,47 @@ int xstateregs_set(struct task_struct *target, const struct user_regset *regset,
> return ret;
> }
>
> +int cetregs_active(struct task_struct *target, const struct user_regset *regset)
> +{
> +#ifdef CONFIG_X86_INTEL_CET
> + if (target->thread.cet.shstk_enabled || target->thread.cet.ibt_enabled)
> + return regset->n;
> +#endif
> + return 0;
> +}
> +
> +int cetregs_get(struct task_struct *target, const struct user_regset *regset,
> + unsigned int pos, unsigned int count,
> + void *kbuf, void __user *ubuf)
> +{
> + struct fpu *fpu = &target->thread.fpu;
> + struct cet_user_state *cetregs;
> +
> + if (!boot_cpu_has(X86_FEATURE_SHSTK))
> + return -ENODEV;
This whole series has a boot_cpu_has, static_cpu_has, and
cpu_feature_enabled all over. Please settle on just one, preferably
static_cpu_has.
> +
> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_MASK_SHSTK_USER);
> +
> + fpu__prepare_read(fpu);
> + return user_regset_copyout(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1);
> +}
> +
> +int cetregs_set(struct task_struct *target, const struct user_regset *regset,
> + unsigned int pos, unsigned int count,
> + const void *kbuf, const void __user *ubuf)
> +{
> + struct fpu *fpu = &target->thread.fpu;
> + struct cet_user_state *cetregs;
> +
> + if (!boot_cpu_has(X86_FEATURE_SHSTK))
> + return -ENODEV;
> +
> + cetregs = get_xsave_addr(&fpu->state.xsave, XFEATURE_MASK_SHSTK_USER);
> +
> + fpu__prepare_write(fpu);
> + return user_regset_copyin(&pos, &count, &kbuf, &ubuf, cetregs, 0, -1);
Is this called for core dumping on current? If so, please make sure
it's correct. (I think it is for get but maybe not for set.)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 05/10] x86/cet: ELF header parsing of Control Flow Enforcement
From: Andy Lutomirski @ 2018-06-07 18:38 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143807.3611-6-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:41 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> Look in .note.gnu.property of an ELF file and check if shadow stack needs
> to be enabled for the task.
Nice! But please structure it so it's one function that parses out
all the ELF notes and some other code (a table or a switch statement)
that handles them. We will probably want to add more kernel-parsed
ELF notes some day, so let's structure the code to make it easier.
> +static int find_cet(u8 *buf, u32 size, u32 align, int *shstk, int *ibt)
> +{
> + unsigned long start = (unsigned long)buf;
> + struct elf_note *note = (struct elf_note *)buf;
> +
> + *shstk = 0;
> + *ibt = 0;
> +
> + /*
> + * Go through the x86_note_gnu_property array pointed by
> + * buf and look for shadow stack and indirect branch
> + * tracking features.
> + * The GNU_PROPERTY_X86_FEATURE_1_AND entry contains only
> + * one u32 as data. Do not go beyond buf_size.
> + */
> +
> + while ((unsigned long) (note + 1) - start < size) {
> + /* Find the NT_GNU_PROPERTY_TYPE_0 note. */
> + if (note->n_namesz == 4 &&
> + note->n_type == NT_GNU_PROPERTY_TYPE_0 &&
> + memcmp(note + 1, "GNU", 4) == 0) {
> + u8 *ptr, *ptr_end;
> +
> + /* Check for invalid property. */
> + if (note->n_descsz < 8 ||
> + (note->n_descsz % align) != 0)
> + return 0;
> +
> + /* Start and end of property array. */
> + ptr = (u8 *)(note + 1) + 4;
> + ptr_end = ptr + note->n_descsz;
Exploitable bug here? You haven't checked that ptr is in bounds or
that ptr + ptr_end is in bounds (or that ptr_end > ptr, for that
matter).
> +
> + while (1) {
> + u32 type = *(u32 *)ptr;
> + u32 datasz = *(u32 *)(ptr + 4);
> +
> + ptr += 8;
> + if ((ptr + datasz) > ptr_end)
> + break;
> +
> + if (type == GNU_PROPERTY_X86_FEATURE_1_AND &&
> + datasz == 4) {
> + u32 p = *(u32 *)ptr;
> +
> + if (p & GNU_PROPERTY_X86_FEATURE_1_SHSTK)
> + *shstk = 1;
> + if (p & GNU_PROPERTY_X86_FEATURE_1_IBT)
> + *ibt = 1;
> + return 1;
> + }
> + }
> + }
> +
> + /*
> + * Note sections like .note.ABI-tag and .note.gnu.build-id
> + * are aligned to 4 bytes in 64-bit ELF objects.
> + */
> + note = (void *)note + ELF_NOTE_NEXT_OFFSET(note, align);
A malicious value here will probably just break out of the while
statement, but it's still scary.
> + }
> +
> + return 0;
> +}
> +
> +static int check_pt_note_segment(struct file *file,
> + unsigned long note_size, loff_t *pos,
> + u32 align, int *shstk, int *ibt)
> +{
> + int retval;
> + char *note_buf;
> +
> + /*
> + * Try to read in the whole PT_NOTE segment.
> + */
> + note_buf = kmalloc(note_size, GFP_KERNEL);
kmalloc() with fully user-controlled, unchecked size is not a good idea.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 3/7] mm/mmap: Add IBT bitmap size to address space limit check
From: Andy Lutomirski @ 2018-06-07 18:39 UTC (permalink / raw)
To: Yu-cheng Yu
Cc: LKML, linux-doc, Linux-MM, linux-arch, X86 ML, H. Peter Anvin,
Thomas Gleixner, Ingo Molnar, H. J. Lu, Shanbhogue, Vedvyas,
Ravi V. Shankar, Dave Hansen, Jonathan Corbet, Oleg Nesterov,
Arnd Bergmann, mike.kravetz
In-Reply-To: <20180607143855.3681-4-yu-cheng.yu@intel.com>
On Thu, Jun 7, 2018 at 7:42 AM Yu-cheng Yu <yu-cheng.yu@intel.com> wrote:
>
> The indirect branch tracking legacy bitmap takes a large address
> space. This causes may_expand_vm() failure on the address limit
> check. For a IBT-enabled task, add the bitmap size to the
> address limit.
>
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@intel.com>
> ---
> arch/x86/include/uapi/asm/resource.h | 5 +++++
> include/uapi/asm-generic/resource.h | 3 +++
> mm/mmap.c | 8 +++++++-
> 3 files changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/uapi/asm/resource.h b/arch/x86/include/uapi/asm/resource.h
> index 04bc4db8921b..0741b2a6101a 100644
> --- a/arch/x86/include/uapi/asm/resource.h
> +++ b/arch/x86/include/uapi/asm/resource.h
> @@ -1 +1,6 @@
> +/* SPDX-License-Identifier: GPL-2.0+ WITH Linux-syscall-note */
> +#ifdef CONFIG_X86_INTEL_CET
> +#define rlimit_as_extra() current->thread.cet.ibt_bitmap_size
> +#endif
> +
> #include <asm-generic/resource.h>
> diff --git a/include/uapi/asm-generic/resource.h b/include/uapi/asm-generic/resource.h
> index f12db7a0da64..8a7608a09700 100644
> --- a/include/uapi/asm-generic/resource.h
> +++ b/include/uapi/asm-generic/resource.h
> @@ -58,5 +58,8 @@
> # define RLIM_INFINITY (~0UL)
> #endif
>
> +#ifndef rlimit_as_extra
> +#define rlimit_as_extra() 0
> +#endif
>
> #endif /* _UAPI_ASM_GENERIC_RESOURCE_H */
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e7d1fcb7ec58..5c07f052bed7 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -3255,7 +3255,13 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> */
> bool may_expand_vm(struct mm_struct *mm, vm_flags_t flags, unsigned long npages)
> {
> - if (mm->total_vm + npages > rlimit(RLIMIT_AS) >> PAGE_SHIFT)
> + unsigned long as_limit = rlimit(RLIMIT_AS);
> + unsigned long as_limit_plus = as_limit + rlimit_as_extra();
> +
> + if (as_limit_plus > as_limit)
> + as_limit = as_limit_plus;
> +
What happens if as_limit_plus overflows? I guess it's probably okay here.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 02/10] x86/cet: Introduce WRUSS instruction
From: Peter Zijlstra @ 2018-06-07 18:41 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Yu-cheng Yu, LKML, linux-doc, Linux-MM, linux-arch, X86 ML,
H. Peter Anvin, Thomas Gleixner, Ingo Molnar, H. J. Lu,
Shanbhogue, Vedvyas, Ravi V. Shankar, Dave Hansen,
Jonathan Corbet, Oleg Nesterov, Arnd Bergmann, mike.kravetz
In-Reply-To: <CALCETrU45Cuzvfz3c1+-+7=9KS2N33Bpp1JqBtaGxhPo8U+Fqg@mail.gmail.com>
On Thu, Jun 07, 2018 at 09:40:02AM -0700, Andy Lutomirski wrote:
> Peterz, isn't there some fancy better way we're supposed to handle the
> error return these days?
Don't think so. I played with a few things but that never really went
anywhere.
Also, both asm things look suspicously similar, it might make sense to
share. Also, maybe do the instruction .byte sequence in a #define INSN
or something.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox