Re: [PATCH SpectreV1+L1TF v4 03/11] config: introduce L1TF_LFENCE option

[Norbert Manthey <nmanthey@amazon.de>]

On 1/23/19 14:18, Jan Beulich wrote:
>>>> On 23.01.19 at 12:51, <nmanthey@amazon.de> wrote:
>> This commit introduces the configuration option L1TF_LFENCE that allows
>> to control the implementation of the protection of privilege checks via
>> lfence instructions. The following four alternatives are provided:
>>
>>  - not injecting lfence instructions
>>  - inject an lfence instruction for both outcomes of the conditional
>>  - inject an lfence instruction only if the conditional would evaluate
>>    to true, so that this case cannot be entered under speculation
> I'd really like to see justification for this variant, as the asymmetric
> handling doesn't look overly helpful. It's also not clear to me how
> someone configuring Xen should tell whether this would be a safe
> selection to make. I'm tempted to request that this option become
> EXPERT dependent.
I will drop this option. Without properly defining which property checks
should be protected (we currently do not protect any XSM based checks
that are used in hypercalls like physdev_op), and what to protect, I
agree it's hard to judge whether this is useful.
>
>>  - evaluating the condition and store the result into a local variable.
>>    before using this value, inject an lfence instruction.
>>
>> The different options allow to control the level of protection vs the
>> slowdown the addtional lfence instructions would introduce. The default
>> value is set to protecting both branches.
>>
>> For non-x86 platforms, the protection is disabled by default.
> At least the "by default" is wrong here.
I will drop the "by default" in this sentence.
>
>> --- a/xen/arch/x86/Kconfig
>> +++ b/xen/arch/x86/Kconfig
>> @@ -176,6 +176,30 @@ config PV_SHIM_EXCLUSIVE
>>  	  firmware, and will not function correctly in other scenarios.
>>  
>>  	  If unsure, say N.
>> +
>> +choice
>> +	prompt "Default L1TF Branch Protection?"
>> +
>> +	config L1TF_LFENCE_BOTH
>> +		bool "Protect both branches of certain conditionals" if HVM
>> +		---help---
>> +		  Inject an lfence instruction after the condition to be
>> +		  evaluated for both outcomes of the condition
>> +	config L1TF_LFENCE_TRUE
>> +		bool "Protect true branch of certain conditionals" if HVM
>> +		---help---
>> +		  Protect only the path where the condition is evaluated to true
>> +	config L1TF_LFENCE_INTERMEDIATE
>> +		bool "Protect before using certain conditionals value" if HVM
>> +		---help---
>> +		  Inject an lfence instruction after evaluating the condition
>> +		  but before forwarding this value from a local variable
>> +	config L1TF_LFENCE_NONE
>> +		bool "No conditional protection"
>> +		---help---
>> +		  Do not inject lfences for conditional evaluations
>> +endchoice
> I guess we should settle on some default for this choice. The
> description talks about a default, but I don't see it set here (and
> I don't think relying merely on the order is a good idea).
I will add a "default" statement, and pick the L1TF_LFENCE_BOTH variant
there.
>
>> --- a/xen/include/xen/nospec.h
>> +++ b/xen/include/xen/nospec.h
>> @@ -68,10 +68,18 @@ static inline bool lfence_true(void) { return true; }
>>  #endif
>>  
>>  /*
>> - * protect evaluation of conditional with respect to speculation
>> + * allow to protect evaluation of conditional with respect to speculation on x86
>>   */
>> -#define evaluate_nospec(condition)                                      \
>> +#if defined(CONFIG_L1TF_LFENCE_NONE) || !defined(CONFIG_X86)
>> +#define evaluate_nospec(condition) (condition)
>> +#elif defined(CONFIG_L1TF_LFENCE_BOTH)
>> +#define evaluate_nospec(condition)                                         \
> I'm puzzled by this line changing - do you really need to move the
> backslash here?

This looks strange as a stand-alone modification, I agree. I will merge
the introduction of the barrier with the new name, and merge it with the
configuration option and the alternative patching. This way, this change
will be removed.

Best,
Norbert

>
> Jan
>
>



Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrer: Christian Schlaeger, Ralf Herbrich
Ust-ID: DE 289 237 879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

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