On 06/02/18 12:30, Zhenzhong Duan wrote: > 在 2018/2/6 19:56, Andrew Cooper 写道: >> On 06/02/18 11:41, Zhenzhong Duan wrote: >>> On 2018/2/6 18:50, Andrew Cooper wrote: >>>> On 06/02/18 10:29, zhenzhong.duan wrote: >>>>> >>>>> >>>>> 2018年2月6日 17:20于 Andrew Cooper <andrew.coop...@citrix.com >>>>> <mailto:andrew.coop...@citrix.com>>写道： >>>>> > >>>>> > On 06/02/2018 09:13, Zhenzhong Duan wrote: >>>>> > > 在 2018/2/6 16:59, Andrew Cooper 写道: >>>>> > >> On 06/02/2018 08:43, Zhenzhong Duan wrote: >>>>> > >>> When ( ibrs && thunk == THUNK_DEFAULT && !retpoline_safe() ) >>>>> is true, >>>>> > >>> thunk is set to THUNK_JMP rather than THUNK_RETPOLINE. >>>>> > >>> >>>>> > >>> When (!ibrs && thunk == THUNK_DEFAULT && !retpoline_safe() ) >>>>> is true, >>>>> > >>> we should do the same. >>>>> > >>> >>>>> > >>> Signed-off-by: Zhenzhong Duan <zhenzhong.d...@oracle.com >>>>> <mailto:zhenzhong.d...@oracle.com>> >>>>> > >> Why? What improvement is this intended to give? >>>>> > > No improvement, I just feel if retpoline isn't safe, THUNK_JMP is >>>>> > > better and safer. >>>>> > > Above first check is working that way. >>>>> > >>>>> > If your only two choices are unsafe repoline or plain jumps, >>>>> then unsafe >>>>> > repoline is far far far safer. >>>>> > >>>>> > Its unsafe properties only kick in on an RSB underflow, and an >>>>> attacker >>>>> > would have to do call-depths analysis of the running binary to >>>>> identify >>>>> > which rets to attempt to poison. >>>>> > >>>>> Thanks for explaining. >>>>> So, for a retpoline safe processor, it just stop using RSB when >>>>> it's empty to avoid underflow? >>>>> >>>> >>>> The qualification of whether a processor is retpoline-safe or not >>>> is whether an RSB underflow causes a BTB lookup (unsafe) or not (safe). >>>> >>>> RSB underflows will always occur; you cannot get rid of them, but >>>> in most cases (i.e. pre-Skylake) they don't fall back to using a >>>> prediction source which can be poisoned by an attacker. >>> Understood. >>>> >>>>> Another question: >>>>> >>>>> if (opt_thunk == THUNK_DEFAULT && opt_ibrs == -1 && >>>>> CONFIG_INDIRECT_THUNK && !cpu_has_lfence_dispatch && >>>>> !retpoline_safe()) >>>>> results in "thunk = THUNK_JMP" regardless of the value of >>>>> "boot_cpu_has(X86_FEATURE_IBRSB)" >>>>> >>>> >>>> Your formatting is hard to follow, >>> Sorry, sent from mobile. >>>> but cpu_has_lfence_dispatch can only be false when virtualised >>>> under an SP2-unaware hypervisor on AMD hardware, at which point >>>> retpoline_safe() will return true. Also, AMD don't have IBRS in >>>> microcode and their future plans don't appear to involve using that >>>> particular CPUID bit. >>> That does make sense for AMD. But what about Intel hardware which >>> has (!cpu_has_lfence_dispatch && !retpoline_safe() && >>> !boot_cpu_has(X86_FEATURE_IBRSB)), should we prefer THUNK_RETPOLINE? >> >> Yes, and we do end up choosing RETPOLINE in those circumstances, as >> we hit the default case you originally tried to patch. > > If that's the case, I think we need another patch like below. > > @@ -223,7 +223,7 @@ void __init init_speculation_mitigations(void) > */ > else if ( retpoline_safe() ) > thunk = THUNK_RETPOLINE; > - else > + else if ( boot_cpu_has(X86_FEATURE_IBRSB) ) > ibrs = true; > } > /* Without compiler thunk support, use IBRS if available. */ > > As ibrs was set to true when !boot_cpu_has(X86_FEATURE_IBRSB) with > current code, then thunk was set to THUNK_JMP rather than THUNK_RETPOLINE.
You are correct. This is a side effect of an optimisation which was requested during review. I'll draft a patch. ~Andrew
_______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
