Re: [Xen-devel] [PATCH v3 2/7] xen/nospec: Use always_inline to fix code gen for evaluate_nospec

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 25/10/2019 13:34, Jan Beulich wrote:
> On 25.10.2019 14:10, Andrew Cooper wrote:
>> On 25/10/2019 13:03, Jan Beulich wrote:
>>> On 23.10.2019 15:58, Andrew Cooper wrote:
>>>> evaluate_nospec() is incredibly fragile, and this is one giant bodge.
>>>>
>>>> To correctly protect jumps, the generated code needs to be of the form:
>>>>
>>>>     cmp/test <cond>
>>>>     jcc 1f
>>>>     lfence
>>>>     ...
>>>>  1: lfence
>>>>     ...
>>>>
>>>> Critically, the lfence must be at the head of both basic blocks, later in 
>>>> the
>>>> instruction stream than the conditional jump in need of protection.
>>>>
>>>> When a static inline is involved, the optimiser decides to be clever and
>>>> rearranges the code as:
>>>>
>>>>  pred:
>>>>     lfence
>>>>     <calculate cond>
>>>>     ret
>>>>
>>>>     call pred
>>>>     cmp $0, %eax
>>>>     jcc 1f
>>>>     ...
>>>>  1: ...
>>>>
>>>> which breaks the speculative safety.
>>> Aiui "pred" is a non-inlined static inline here.
>> Correct, although it actually applies to anything which the compiler
>> chose to out of line, perhaps even as a side effect of CSE pass.
> Not sure if you're alluding to such, but I've never seen the compiler
> out-of-line something that wasn't a function (or perhaps a specialization
> of one) at the source level.

I've seen it with LTO in both Clang and GCC, where the compilers can
safely reason about the lack of side effects in function calls.

>
>>>> This is the transitive set of predicates which I can spot which need
>>>> protecting.  There are probably ones I've missed.  Personally, I'm -1 for 
>>>> this
>>>> approach, but the only other option for 4.13 is to revert it all to unbreak
>>>> livepatching.
>>> To unbreak livepatching, aiui what you need is symbol disambiguation,
>>> a patch for which has been sent.
>> Correct, but..
>>
>>> With this I think we should focus on
>>> code generation aspects here. I'm fine ack-ing the code changes with
>>> a modified description. But since you're -1 for this, I'm not sure in
>>> the first place that we want to go this route.
>> ... without this change, l1tf-barrier/branch-hardening is still broken,
>> and a performance overhead.
> Well, it has less of an effect, but it's still better than without any
> of this altogether.

I certainly don't agree with this conclusion.

> In some cases code generation is correct,

I agree with this, but ...

> and in some other cases code generation is at least such that the window size
> gets shrunk.

... this isn't accurate.  In the case that out-of-lining happens, you
get an lfence earlier in the instruction stream, which serialises an
unrelated bit of logic (hence the perf hit), and does nothing for the
speculation window which the evaluate_nospec() was intending to protect.

>
>> The two choices to unblock 4.13 are this patch, or the previous version
>> which made CONFIG_HARDEN_BRANCH depend on BROKEN, which was even more
>> disliked.
> Option 3 is to have just the config option, for people to turn this
> off if they feel like doing so.

Yes, but no.  A facade of security is worse than no security, and I
don't consider doing that an acceptable solution in this case.

~Andrew

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