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

[Norbert Manthey <nmanthey@xxxxxxxxx>]

On 10/28/19 18:05, Andrew Cooper wrote:
> On 25/10/2019 22:56, Norbert Manthey wrote:
>> On 10/25/19 17:40, Jan Beulich wrote:
>>> On 25.10.2019 17:27, Andrew Cooper wrote:
>>>> On 25/10/2019 13:34, Jan Beulich wrote:
>>>>> On 25.10.2019 14:10, Andrew Cooper wrote:
>>>>>> 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.
>>> But I thought we all agree that this is something that's presumably
>>> going to remain incomplete (as in not provably complete) altogether
>>> anyway. It's just that without the change here it's far more
>>> incomplete then with it.
> This is inherently a best-effort approach, but without the
> always_inline, it is tantamount to useless.
>
> Only the grant table operations, and __mfn_valid() are correctly
> protected with the code in its current form, where as the large changes
> (in terms of absolute number of protected paths) comes from the predicates.
>
>>> In any event I think we should (also) have an opinion from the people
>>> who had originally contributed this logic. You didn't Cc anyone of
>>> them; I've added at least Norbert now.
>> Thanks for adding me. I had a quick look into the discussion. Only
>> making adding lfence statements around conditionals depending on config
>> BROKEN does not help, as it would still need to be always_inline to work
>> as expected, correct?
> "depends on BROKEN" is a way to unconditionally compile out
> functionality, which was one option considered to this problem.
>
>> Hence, in my opinion, this patch does the right
>> thing to benefit from the lfences that are placed after evaluation
>> conditionals.
> This patch is the alternative to compiling everything out.
>
> I'm still holding out hope that we'll find a better compiler based
> mitigation in the longer term, because I don't see either of these
> options being viable strategies longterm.

I fully agree that in the long run, we should have compiler support, to
e.g. not move lfence statements around.

However, until we have that, we should allow users of Xen to get the
lfence statements at the correct positions as a best effort practice.
Hence, the always_inline modification should be there. In case you still
want to compile out this functionality, you could even add a dependency
on BROKEN on top, and then users can chose to compile it in, but at
least get a version where the lfences are placed at the right position.

Best,
Norbert

>
> ~Andrew




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


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