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 29/10/2019 08:25, Norbert Manthey wrote:
> 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.

This is going around in circles.

The following patch in this series is a fully user-selectable Kconfig
option for whether they want to use branch hardening, and is in place
once there is a plausible expectation that the lfences are in suitable
positions.

If this patch series does not agreement, I will unblock livepatching on
4.13 by committing the v2 patch which causes BRANCH_HARDEN to depend on
BROKEN and force it to be compiled out with no user choice at all.

Unbreaking livepatching is strictly more important than keeping a brand
new feature in 4.13 in a broken form.  I've provided two alternative
strategies to fix the 4.13 blockers, but if noone can agree on which
approach to use, I will commit the simpler fix.

~Andrew

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