Re: [Xen-devel] [PATCH for-4.13 2/2] xen/nospec: Introduce CONFIG_SPECULATIVE_BRANCH_HARDEN and disable it

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 30/09/2019 21:17, Julien Grall wrote:
> Hi,
>
> On 9/30/19 7:24 PM, Andrew Cooper wrote:
>> The code generation for barrier_nospec_true() is not correct.  We are
>> taking a
>> perf it from the added fences, but not gaining any speculative safety.
>
> s/it/hit/?

Yes.

>
>>
>> This is caused by inline assembly trying to fight the compiler
>> optimiser, and
>> the optimiser winning.  There is no clear way to achieve safety, so
>> turn the
>> perf hit off for now.
>>
>> This also largely reverts 3860d5534df4.  The name 'l1tf-barrier', and
>> making
>> barrier_nospec_true() depend on CONFIG_HVM was constrained by what
>> could be
>> discussed publicly at the time.  Now that MDS is public, neither
>> aspects are
>> correct.
>>
>> As l1tf-barrier hasn't been in a release of Xen, and
>> CONFIG_SPECULATIVE_BRANCH_HARDEN is disabled until we can find a safe
>> way of
>> implementing the functionality, remove the l1tf-barrier command line
>> option.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Wei Liu <wl@xxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> CC: Juergen Gross <jgross@xxxxxxxx>
>> CC: Norbert Manthey <nmanthey@xxxxxxxxx>
>> ---
>>   docs/misc/xen-command-line.pandoc |  8 +-------
>>   xen/arch/x86/spec_ctrl.c          | 17 ++---------------
>>   xen/common/Kconfig                | 17 +++++++++++++++++
>
> I think this wanted to have "THE REST" CCed.
>
>>   xen/include/asm-x86/cpufeatures.h |  2 +-
>>   xen/include/asm-x86/nospec.h      |  4 ++--
>>   xen/include/asm-x86/spec_ctrl.h   |  1 -
>>   6 files changed, 23 insertions(+), 26 deletions(-)
>
> [...]
>
>> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
>> index 9644cc9911..d851e63083 100644
>> --- a/xen/common/Kconfig
>> +++ b/xen/common/Kconfig
>> @@ -96,6 +96,23 @@ config SPECULATIVE_ARRAY_HARDEN
>>           If unsure, say Y.
>>   +config SPECULATIVE_BRANCH_HARDEN
>> +    bool "Speculative Branch Hardening"
>> +    depends on BROKEN
>> +        ---help---
>> +      Contemporary processors may use speculative execution as a
>> +      performance optimisation, but this can potentially be abused
>> by an
>> +      attacker to leak data via speculative sidechannels.
>> +
>> +      One source of misbehaviour is by executing the wrong basic block
>> +      following a conditional jump.
>> +
>> +      When enabled, specific conditions which have been deemed
>> liable to
>> +      be speculatively abused will be hardened to avoid entering the
>> wrong
>> +      basic block.
>> +
>> +      !!! WARNING - This is broken and doesn't generate safe code !!!
>
> Any reason to add that in common code when this is x86 only?

In principle, its not x86 specific.

> My worry is this gate config gate nothing on Arm so the user may have
> a false sense that it can be used (even though it is clearly BROKEN).
>
> Also the name is quite close to the CONFIG_HARDEN_PREDICTOR on Arm and
> may confuse user. Although, I don't have a better name so far :/

The "depends on BROKEN" means it will never show up to a user in
menuconfig, which is why it was only CC to x86, and not to rest.

As for naming, I'm open to suggestions, but this was the best I could
come up with.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel