linux-kernel - Re: [PATCH v2 06/19] asm-generic/barrier: mask speculative execution flows

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: <20180112091246.GP32035@hirez.programming.kicks-ass.net>
Date:   Fri, 12 Jan 2018 10:12:46 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Dan Williams <dan.j.williams@...el.com>
Cc:     linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
        kernel-hardening@...ts.openwall.com,
        Catalin Marinas <catalin.marinas@....com>, x86@...nel.org,
        Will Deacon <will.deacon@....com>,
        Alexei Starovoitov <ast@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Ingo Molnar <mingo@...hat.com>,
        "H. Peter Anvin" <hpa@...or.com>, tglx@...utronix.de,
        torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
        alan@...ux.intel.com
Subject: Re: [PATCH v2 06/19] asm-generic/barrier: mask speculative execution
 flows

On Thu, Jan 11, 2018 at 04:46:56PM -0800, Dan Williams wrote:
> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
> new file mode 100644
> index 000000000000..5c66fc30f919
> --- /dev/null
> +++ b/include/linux/nospec.h
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2018 Intel Corporation. All rights reserved.
> +
> +#ifndef __NOSPEC_H__
> +#define __NOSPEC_H__
> +
> +#include <linux/jump_label.h>
> +#include <asm/barrier.h>
> +
> +#ifndef array_ptr_mask
> +#define array_ptr_mask(idx, sz)						\
> +({									\
> +	unsigned long mask;						\
> +	unsigned long _i = (idx);					\
> +	unsigned long _s = (sz);					\
> +									\
> +	mask = ~(long)(_i | (_s - 1 - _i)) >> (BITS_PER_LONG - 1);	\
> +	mask;								\
> +})
> +#endif
> +
> +/**
> + * __array_ptr - Generate a pointer to an array element, ensuring
> + * the pointer is bounded under speculation to NULL.
> + *
> + * @base: the base of the array
> + * @idx: the index of the element, must be less than LONG_MAX
> + * @sz: the number of elements in the array, must be less than LONG_MAX
> + *
> + * If @idx falls in the interval [0, @sz), returns the pointer to
> + * @arr[@idx], otherwise returns NULL.
> + */
> +#define __array_ptr(base, idx, sz)					\
> +({									\
> +	union { typeof(*(base)) *_ptr; unsigned long _bit; } __u;	\
> +	typeof(*(base)) *_arr = (base);					\
> +	unsigned long _i = (idx);					\
> +	unsigned long _mask = array_ptr_mask(_i, (sz));			\
> +									\
> +	__u._ptr = _arr + (_i & _mask);					\
> +	__u._bit &= _mask;						\
> +	__u._ptr;							\
> +})
> +
> +#ifdef CONFIG_SPECTRE1_IFENCE
> +DECLARE_STATIC_KEY_TRUE(nospec_key);
> +#else
> +DECLARE_STATIC_KEY_FALSE(nospec_key);
> +#endif
> +
> +#ifdef ifence_array_ptr
> +/*
> + * The expectation is that no compiler or cpu will mishandle __array_ptr
> + * leading to problematic speculative execution. Bypass the ifence
> + * based implementation by default.
> + */
> +#define array_ptr(base, idx, sz)				\
> +({								\
> +	typeof(*(base)) *__ret;					\
> +								\
> +	if (static_branch_unlikely(&nospec_key))		\
> +		__ret = ifence_array_ptr(base, idx, sz);	\
> +	else							\
> +		__ret = __array_ptr(base, idx, sz);		\
> +	__ret;							\
> +})


So I think this wants:

#ifndef HAVE_JUMP_LABEL
#error Compiler lacks asm-goto, can generate unsafe code
#endif

Suppose the generic array_ptr_mask() is unsafe on some arch and they
only implement ifence_array_ptr() and they compile without asm-goto,
then the above reverts to a dynamic condition, which can be speculated.
If we then speculate into the 'bad' __array_ptr we're screwed.

> +#else
> +#define array_ptr __array_ptr
> +#endif
> +
> +#endif /* __NOSPEC_H__ */


In general I think I would write all this in a form like:

#define __array_ptr(base, idx, sz)					\
({									\
	union { typeof(*(base)) *_ptr; unsigned long _bit; } __u;	\
	typeof(*(base)) *_arr = (base);					\
	unsigned long _i = (idx);					\
	unsigned long _mask = array_ptr_mask(_i, (sz));			\
									\
	__u._ptr = _arr + (_i & _mask);					\
	__u._bit &= _mask;						\
	__u._ptr;							\
})

#if defined(array_ptr_mask) && defined(ifence_array_ptr)

#ifndef HAVE_JUMP_LABEL
#error Compiler lacks asm-goto, can generate unsafe code
#endif

#define array_ptr(base, idx, sz)				\
({								\
	typeof(*(base)) *__ret;					\
								\
	if (static_branch_unlikely(&nospec_key))		\
		__ret = ifence_array_ptr(base, idx, sz);	\
	else							\
		__ret = __array_ptr(base, idx, sz);		\
	__ret;							\
})

#elif defined(array_ptr_mask)

#define array_ptr(base, idx, sz) __array_ptr(base, idx, sz)

#elif defined(ifence_array_ptr)

#define array_ptr(base, idx, sz) ifence_array_ptr(base, idx, sz)

#else

/* XXX we want a suitable warning here ? */

#define array_ptr(base, idx, sz) (idx < sz ? base + idx : NULL)

#endif

and stick the generic array_ptr_mask into asm-generic/nospec.h or
something.

Then the static key stuff is limited to architectures that define _both_
array_ptr_mask and ifence_array_ptr.