linux-kernel - Re: [PATCH v23 22/28] x86/cet/shstk: User-mode shadow stack support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20210318123215.GE19570@zn.tnic>
Date:   Thu, 18 Mar 2021 13:32:15 +0100
From:   Borislav Petkov <bp@...en8.de>
To:     Yu-cheng Yu <yu-cheng.yu@...el.com>
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>,
        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 v23 22/28] x86/cet/shstk: User-mode shadow stack support

> Subject: Re: [PATCH v23 22/28] x86/cet/shstk:   User-mode shadow stack support
						^
						Add

On Tue, Mar 16, 2021 at 08:10:48AM -0700, Yu-cheng Yu wrote:
> Introduce basic shadow stack enabling/disabling/allocation routines.
> A task's shadow stack is allocated from memory with VM_SHSTK flag and has
> a fixed size of min(RLIMIT_STACK, 4GB).
> 
> Signed-off-by: Yu-cheng Yu <yu-cheng.yu@...el.com>
> Reviewed-by: Kees Cook <keescook@...omium.org>
> ---
>  arch/x86/include/asm/cet.h       |  28 ++++++
>  arch/x86/include/asm/processor.h |   5 ++
>  arch/x86/kernel/Makefile         |   2 +
>  arch/x86/kernel/cet.c            | 147 +++++++++++++++++++++++++++++++

Yeah, since Peter wants stuff split, let's call that shstk.c and the IBT
stuff goes into a separate ibt.c please.

> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index dc6d149bf851..3fce5062261b 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -27,6 +27,7 @@ struct vm86;
>  #include <asm/unwind_hints.h>
>  #include <asm/vmxfeatures.h>
>  #include <asm/vdso/processor.h>
> +#include <asm/cet.h>
>  
>  #include <linux/personality.h>
>  #include <linux/cache.h>
> @@ -535,6 +536,10 @@ struct thread_struct {
>  
>  	unsigned int		sig_on_uaccess_err:1;
>  
> +#ifdef CONFIG_X86_CET
> +	struct cet_status	cet;

	struct shstk_desc	shstk;

or so.

> +int cet_setup_shstk(void)
> +{
> +	unsigned long addr, size;
> +	struct cet_status *cet = &current->thread.cet;
> +
> +	if (!static_cpu_has(X86_FEATURE_SHSTK))

cpu_feature_enabled

> +		return -EOPNOTSUPP;
> +
> +	size = round_up(min(rlimit(RLIMIT_STACK), 1UL << 32), PAGE_SIZE);
						  ^
						  SZ_4G

> +	addr = alloc_shstk(size, 0);
> +

^ Superfluous newline.

> +	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 cet_disable_shstk(void)
> +{
> +	struct cet_status *cet = &current->thread.cet;
> +	u64 msr_val;
> +
> +	if (!static_cpu_has(X86_FEATURE_SHSTK) ||

cpu_feature_enabled

And put the || on the end of each line:

	if (!cpu_feature_enabled() ||
	    !cet->shstk_size ||
	    ... )


> +	    !cet->shstk_size || !cet->shstk_base)
> +		return;
> +
> +	start_update_msrs();
> +	rdmsrl(MSR_IA32_U_CET, msr_val);
> +	wrmsrl(MSR_IA32_U_CET, msr_val & ~CET_SHSTK_EN);
> +	wrmsrl(MSR_IA32_PL3_SSP, 0);
> +	end_update_msrs();
> +
> +	cet_free_shstk(current);
> +}

Put that function under cet_free_shstk().

> +void cet_free_shstk(struct task_struct *tsk)
> +{
> +	struct cet_status *cet = &tsk->thread.cet;
> +
> +	if (!static_cpu_has(X86_FEATURE_SHSTK) ||

cpu_feature_enabled and as above.

> +	    !cet->shstk_size || !cet->shstk_base)
> +		return;
> +
> +	if (!tsk->mm || tsk->mm != current->mm)
> +		return;

You're operating on current here merrily but what's protecting all those
paths operating on current from getting current changed underneath them
due to scheduling? IOW, is preemption safely disabled in all those
paths ending up here?

> +
> +	while (1) {

Uuh, an endless loop. What guarantees we'll exit it relatively timely...

> +		int r;
> +
> +		r = vm_munmap(cet->shstk_base, cet->shstk_size);
> +
> +		/*
> +		 * Retry if mmap_lock is not available.
> +		 */
> +		if (r == -EINTR) {
> +			cond_resched();

... that thing?

> +			continue;
> +		}
> +
> +		WARN_ON_ONCE(r);
> +		break;
> +	}
> +
> +	cet->shstk_base = 0;
> +	cet->shstk_size = 0;
> +}
> -- 
> 2.21.0
> 

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette