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@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
