kernel-hardening - Re: [PATCH v5 04/12] x86: introduce __uaccess_begin

Follow @Openwall on Twitter for new release announcements and other news

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180128090652.344ji6p6yapre6ej@gmail.com>
Date: Sun, 28 Jan 2018 10:06:52 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Dan Williams <dan.j.williams@...el.com>
Cc: tglx@...utronix.de, linux-arch@...r.kernel.org,
	Tom Lendacky <thomas.lendacky@....com>,
	Andi Kleen <ak@...ux.intel.com>, Kees Cook <keescook@...omium.org>,
	kernel-hardening@...ts.openwall.com, gregkh@...uxfoundation.org,
	x86@...nel.org, Ingo Molnar <mingo@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>, "H. Peter Anvin" <hpa@...or.com>,
	torvalds@...ux-foundation.org, alan@...ux.intel.com
Subject: Re: [PATCH v5 04/12] x86: introduce __uaccess_begin_nospec and ifence


* Dan Williams <dan.j.williams@...el.com> wrote:

> For '__get_user' paths, do not allow the kernel to speculate on the
> value of a user controlled pointer. In addition to the 'stac'
> instruction for Supervisor Mode Access Protection, an 'ifence' causes
> the 'access_ok' result to resolve in the pipeline before the cpu might
> take any speculative action on the pointer value.
> 
> Since __get_user is a major kernel interface that deals with user
> controlled pointers, the '__uaccess_begin_nospec' mechanism will prevent
> speculative execution past an 'access_ok' permission check. While
> speculative execution past 'access_ok' is not enough to lead to a kernel
> memory leak, it is a necessary precondition.
> 
> To be clear, '__uaccess_begin_nospec' is addressing a class of potential
> problems near '__get_user' usages.
> 
> Note, that while ifence is used to protect '__get_user', pointer masking
> will be used for 'get_user' since it incorporates a bounds check near
> the usage.
> 
> There are no functional changes in this patch.

The style problems/inconsistencies of the #2 patch are repeated here too.

Also, please split this patch into two patches:

 - #1 introducing ifence() and using it where it was open coded before
 - #2 introducing the _nospec() uaccess variants

> Suggested-by: Linus Torvalds <torvalds@...ux-foundation.org>
> Suggested-by: Andi Kleen <ak@...ux.intel.com>
> Cc: Tom Lendacky <thomas.lendacky@....com>
> Cc: Al Viro <viro@...iv.linux.org.uk>
> Cc: Kees Cook <keescook@...omium.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: "H. Peter Anvin" <hpa@...or.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: x86@...nel.org
> Signed-off-by: Dan Williams <dan.j.williams@...el.com>
> ---
>  arch/x86/include/asm/barrier.h |    4 ++++
>  arch/x86/include/asm/msr.h     |    3 +--
>  arch/x86/include/asm/uaccess.h |    9 +++++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/barrier.h b/arch/x86/include/asm/barrier.h
> index 30419b674ebd..5f11d4c5c862 100644
> --- a/arch/x86/include/asm/barrier.h
> +++ b/arch/x86/include/asm/barrier.h
> @@ -46,6 +46,10 @@ static inline unsigned long array_idx_mask(unsigned long idx, unsigned long sz)
>  	return mask;
>  }
>  
> +/* prevent speculative execution past this barrier */

Please use consistent capitalization in comments.

Thanks,

	Ingo

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.