On Sun, Jan 28, 2018 at 12:55 AM, Ingo Molnar <mingo@xxxxxxxxxx> wrote:
>
> Firstly, I only got a few patches of this series so I couldn't review all of them
> - please Cc: me to all future Meltdown and Spectre related patches!
>
> * Dan Williams <dan.j.williams@xxxxxxxxx> wrote:
>
>> 'array_idx' is proposed as a generic mechanism to mitigate against
>> Spectre-variant-1 attacks, i.e. an attack that bypasses boundary checks
>> via speculative execution). The 'array_idx' implementation is expected
>> to be safe for current generation cpus across multiple architectures
>> (ARM, x86).
>
> nit: Stray closing parenthesis
>
> s/cpus/CPUs
>
>> Based on an original implementation by Linus Torvalds, tweaked to remove
>> speculative flows by Alexei Starovoitov, and tweaked again by Linus to
>> introduce an x86 assembly implementation for the mask generation.
>>
>> Co-developed-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
>> Co-developed-by: Alexei Starovoitov <ast@xxxxxxxxxx>
>> Suggested-by: Cyril Novikov <cnovikov@xxxxxxxx>
>> Cc: Russell King <linux@xxxxxxxxxxxxxxx>
>> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will.deacon@xxxxxxx>
>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>> Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
>> Cc: x86@xxxxxxxxxx
>> Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
>> ---
>> include/linux/nospec.h | 64 ++++++++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 64 insertions(+)
>> create mode 100644 include/linux/nospec.h
>>
>> diff --git a/include/linux/nospec.h b/include/linux/nospec.h
>> new file mode 100644
>> index 000000000000..f59f81889ba3
>> --- /dev/null
>> +++ b/include/linux/nospec.h
>> @@ -0,0 +1,64 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright(c) 2018 Intel Corporation. All rights reserved.
>
> Given the close similarity of Linus's array_access() prototype pseudocode there
> should probably also be:
>
> Copyright (C) 2018 Linus Torvalds
>
> in that file?
Yes, and Alexei as well.
>
>> +
>> +#ifndef __NOSPEC_H__
>> +#define __NOSPEC_H__
>> +
>> +/*
>> + * When idx is out of bounds (idx >= sz), the sign bit will be set.
>> + * Extend the sign bit to all bits and invert, giving a result of zero
>> + * for an out of bounds idx, or ~0UL if within bounds [0, sz).
>> + */
>> +#ifndef array_idx_mask
>> +static inline unsigned long array_idx_mask(unsigned long idx, unsigned long sz)
>> +{
>> + /*
>> + * Warn developers about inappropriate array_idx usage.
>> + *
>> + * Even if the cpu speculates past the WARN_ONCE branch, the
>
> s/cpu/CPU
>
>> + * sign bit of idx is taken into account when generating the
>> + * mask.
>> + *
>> + * This warning is compiled out when the compiler can infer that
>> + * idx and sz are less than LONG_MAX.
>
> Please use 'idx' and 'sz' in quotes, to make sure they stand out more in free
> flowing comment text. Also please use '()' to denote functions/methods.
>
> I.e. something like:
>
> * Warn developers about inappropriate array_idx() usage.
> *
> * Even if the CPU speculates past the WARN_ONCE() branch, the
> * sign bit of 'idx' is taken into account when generating the
> * mask.
> *
> * This warning is compiled out when the compiler can infer that
> * 'idx' and 'sz' are less than LONG_MAX.
>
> That's just one example - please apply it to all comments consistently.
>
>> + */
>> + if (WARN_ONCE(idx > LONG_MAX || sz > LONG_MAX,
>> + "array_idx limited to range of [0, LONG_MAX]\n"))
>
> Same in user facing messages:
>
> "array_idx() limited to range of [0, LONG_MAX]\n"))
>
>> + * For a code sequence like:
>> + *
>> + * if (idx < sz) {
>> + * idx = array_idx(idx, sz);
>> + * val = array[idx];
>> + * }
>> + *
>> + * ...if the cpu speculates past the bounds check then array_idx() will
>> + * clamp the index within the range of [0, sz).
>
> s/cpu/CPU
>
>> + */
>> +#define array_idx(idx, sz) \
>> +({ \
>> + typeof(idx) _i = (idx); \
>> + typeof(sz) _s = (sz); \
>> + unsigned long _mask = array_idx_mask(_i, _s); \
>> + \
>> + BUILD_BUG_ON(sizeof(_i) > sizeof(long)); \
>> + BUILD_BUG_ON(sizeof(_s) > sizeof(long)); \
>> + \
>> + _i &= _mask; \
>> + _i; \
>> +})
>> +#endif /* __NOSPEC_H__ */
>
> For heaven's sake, please name a size variable as 'size', not 'sz'. We don't have
> a shortage of characters and can deobfuscate common primitives, can we?
>
> Also, beyond the nits, I also hate the namespace here. We have a new generic
> header providing two new methods:
>
> #include <linux/nospec.h>
>
> array_idx_mask()
> array_idx()
>
> which is then optimized for x86 in asm/barrier.h. That's already a non-sequitor.
>
> Then we introduce uaccess API variants with a _nospec() postfix.
>
> Then we add ifence() to x86.
>
> There's no naming coherency to this.
Ingo, I love you, but please take the incredulity down a bit,
especially when I had 'nospec' in all the names in v1. Thomas, Peter,
and Alexei wanted s/nospec_barrier/ifence/ and
s/array_idx_nospec/array_idx/. You can always follow on with a patch
to fix up the names and placements to your liking. While they'll pick
on my name choices, they won't pick on yours, because I simply can't
be bothered to care about a bikeshed color at this point after being
bounced around for 5 revisions of this patch set.
> A better approach would be to signal the 'no speculation' aspect of the
> array_idx() methods already: naming it array_idx_nospec() would be a solution,
> as it clearly avoids speculation beyond the array boundaries.
>
> Also, without seeing the full series it's hard to tell, whether the introduction
> of linux/nospec.h is justified, but it feels somewhat suspect.
>
