linux-kernel - Re: [PATCH v5 06/12] x86, get_user: use pointer masking to limit speculation

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>] [day] [month] [year] [list]

Date:   Sun, 28 Jan 2018 10:25:28 +0100
From:   Ingo Molnar <mingo@...nel.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     tglx@...utronix.de, linux-arch@...r.kernel.org,
        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>,
        Andy Lutomirski <luto@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>, torvalds@...ux-foundation.org,
        alan@...ux.intel.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 06/12] x86, get_user: use pointer masking to limit
 speculation


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

> Quoting Linus:
> 
>     I do think that it would be a good idea to very expressly document
>     the fact that it's not that the user access itself is unsafe. I do
>     agree that things like "get_user()" want to be protected, but not
>     because of any direct bugs or problems with get_user() and friends,
>     but simply because get_user() is an excellent source of a pointer
>     that is obviously controlled from a potentially attacking user
>     space. So it's a prime candidate for then finding _subsequent_
>     accesses that can then be used to perturb the cache.
> 
> Unlike the '__get_user' case 'get_user' includes the address limit check
> near the pointer de-reference. With that locality the speculation can be
> mitigated with pointer narrowing rather than a barrier. Where the
> narrowing is performed by:
> 
> 	cmp %limit, %ptr
> 	sbb %mask, %mask
> 	and %mask, %ptr
> 
> With respect to speculation the value of %ptr is either less than %limit
> or NULL.

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

> --- a/arch/x86/lib/getuser.S
> +++ b/arch/x86/lib/getuser.S
> @@ -40,6 +40,8 @@ ENTRY(__get_user_1)
>  	mov PER_CPU_VAR(current_task), %_ASM_DX
>  	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
>  	jae bad_get_user
> +	sbb %_ASM_DX, %_ASM_DX		/* 0 - (uptr < addr_limit) */
> +	and %_ASM_DX, %_ASM_AX
>  	ASM_STAC
>  1:	movzbl (%_ASM_AX),%edx
>  	xor %eax,%eax
> @@ -54,6 +56,8 @@ ENTRY(__get_user_2)
>  	mov PER_CPU_VAR(current_task), %_ASM_DX
>  	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
>  	jae bad_get_user
> +	sbb %_ASM_DX, %_ASM_DX		/* 0 - (uptr < addr_limit) */
> +	and %_ASM_DX, %_ASM_AX
>  	ASM_STAC
>  2:	movzwl -1(%_ASM_AX),%edx
>  	xor %eax,%eax
> @@ -68,6 +72,8 @@ ENTRY(__get_user_4)
>  	mov PER_CPU_VAR(current_task), %_ASM_DX
>  	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
>  	jae bad_get_user
> +	sbb %_ASM_DX, %_ASM_DX		/* 0 - (uptr < addr_limit) */
> +	and %_ASM_DX, %_ASM_AX
>  	ASM_STAC
>  3:	movl -3(%_ASM_AX),%edx
>  	xor %eax,%eax
> @@ -83,6 +89,8 @@ ENTRY(__get_user_8)
>  	mov PER_CPU_VAR(current_task), %_ASM_DX
>  	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
>  	jae bad_get_user
> +	sbb %_ASM_DX, %_ASM_DX		/* 0 - (uptr < addr_limit) */
> +	and %_ASM_DX, %_ASM_AX
>  	ASM_STAC
>  4:	movq -7(%_ASM_AX),%rdx
>  	xor %eax,%eax
> @@ -94,6 +102,8 @@ ENTRY(__get_user_8)
>  	mov PER_CPU_VAR(current_task), %_ASM_DX
>  	cmp TASK_addr_limit(%_ASM_DX),%_ASM_AX
>  	jae bad_get_user_8
> +	sbb %_ASM_DX, %_ASM_DX		/* 0 - (uptr < addr_limit) */
> +	and %_ASM_DX, %_ASM_AX

Please make it clear in the comments that these are essentially open-coded 
assembly versions of array_idx_mask_nospec(), that the purpose here is to provide 
as a partial speculation barrier against the value range of user-provided 
pointers.

In a couple of years this sequence will be too obscure to understand at first 
glance.

It would also make it easier to find these spots if someone wants to tweak (or 
backport) array_idx_mask_nospec() related changes.

Thanks,

	Ingo