[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 12/22] x86/spec-ctrl: introduce Address Space Isolation command line option
On Wed, Sep 25, 2024 at 04:03:04PM +0200, Jan Beulich wrote: > On 25.09.2024 15:31, Roger Pau Monné wrote: > > On Wed, Aug 14, 2024 at 12:10:56PM +0200, Jan Beulich wrote: > >> On 26.07.2024 17:21, Roger Pau Monne wrote: > >>> --- a/docs/misc/xen-command-line.pandoc > >>> +++ b/docs/misc/xen-command-line.pandoc > >>> @@ -2387,7 +2387,7 @@ By default SSBD will be mitigated at runtime (i.e > >>> `ssbd=runtime`). > >>> > >>> ### spec-ctrl (x86) > >>> > `= List of [ <bool>, xen=<bool>, {pv,hvm}=<bool>, > >>> -> {msr-sc,rsb,verw,{ibpb,bhb}-entry}=<bool>|{pv,hvm}=<bool>, > >>> +> > >>> {msr-sc,rsb,verw,{ibpb,bhb}-entry,asi}=<bool>|{pv,hvm}=<bool>, > >> > >> Is it really appropriate to hide this underneath an x86-only option? Even > >> of other architectures won't support it right away, they surely will want > >> to down the road? In which case making as much of this common right away > >> is probably the best we can do. This goes along with the question whether, > >> like e.g. "xpti", this should be a top-level option. > > > > I think it's better placed in spec-ctrl as it's a speculation > > mitigation. > > As is XPTI. But XPTI predates the introduction of spec-ctrl option, I assumed that's why xpti is not part of spec-ctrl. > > I can see your point about sharing with other arches, > > maybe when that's needed we can introduce a generic parser of > > spec-ctrl options? > > Not sure how much could be generalized there. Oh, so your point was not about sharing the parsing code, but sharing the command line documentation about it, sorry, I missed that. Along the lines of: asi= boolean | { pv, hvm, hwdom } Or similar? Even then sub-options would likely be different between architectures. > >>> @@ -143,6 +148,10 @@ static int __init cf_check parse_spec_ctrl(const > >>> char *s) > >>> opt_unpriv_mmio = false; > >>> opt_gds_mit = 0; > >>> opt_div_scrub = 0; > >>> + > >>> + opt_asi_pv = 0; > >>> + opt_asi_hwdom = 0; > >>> + opt_asi_hvm = 0; > >>> } > >>> else if ( val > 0 ) > >>> rc = -EINVAL; > >> > >> I'm frequently in trouble when deciding where the split between "=no" and > >> "=xen" should be. opt_xpti_* are cleared ahead of the disable_common label; > >> considering the similarity I wonder whether the same should be true for ASI > >> (as this is also or even mainly about protecting guests from one another), > >> or whether the XPTI placement is actually wrong. > > > > Hm, that's a difficult one. ASI is a Xen implemented mitigation, so > > it should be turned off when spec-ctrl=no-xen is used according to the > > description of the option: > > > > "spec-ctrl=no-xen can be used to turn off all of Xen’s mitigations" > > Meaning (aiui) mitigations to protect Xen itself. So that would speculation attacks that take place in Xen context, which is what ASI would protect against? I don't have a strong opinion, but I also have a hard time seeing what should `no-xen` disable. > >>> @@ -378,6 +410,13 @@ int8_t __ro_after_init opt_xpti_domu = -1; > >>> > >>> static __init void xpti_init_default(void) > >>> { > >>> + ASSERT(opt_asi_pv >= 0 && opt_asi_hwdom >= 0); > >>> + if ( (opt_xpti_hwdom == 1 || opt_xpti_domu == 1) && opt_asi_pv == 1 ) > >> > >> There is a separate opt_asi_hwdom which isn't used here, but only ... > > > > opt_asi_pv (and opt_asi_hvm) must be set for opt_asi_hwdom to also be > > set. XPTI is sligtly different, in that XPTI could be set only for > > the hwdom by using `xpti=dom0`. > > Hmm, I didn't even notice this oddity (as it feels to me) in parsing. > From the doc provided it wouldn't occur to me that e.g. "asi=pv" won't > affect a PV Dom0. That's (iirc) specifically why "xpti=" has a "hwdom" > sub-option. It seems to be like that for all spec-ctrl options, see `bhb-entry` for example. > >>> @@ -643,22 +683,24 @@ static void __init print_details(enum ind_thunk > >>> thunk) > >>> opt_eager_fpu ? " EAGER_FPU" > >>> : "", > >>> opt_verw_hvm ? " VERW" > >>> : "", > >>> boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) ? " IBPB-entry" > >>> : "", > >>> - opt_bhb_entry_hvm ? " BHB-entry" > >>> : ""); > >>> + opt_bhb_entry_hvm ? " BHB-entry" > >>> : "", > >>> + opt_asi_hvm ? " ASI" > >>> : ""); > >>> > >>> #endif > >>> #ifdef CONFIG_PV > >>> - printk(" Support for PV VMs:%s%s%s%s%s%s%s\n", > >>> + printk(" Support for PV VMs:%s%s%s%s%s%s%s%s\n", > >>> (boot_cpu_has(X86_FEATURE_SC_MSR_PV) || > >>> boot_cpu_has(X86_FEATURE_SC_RSB_PV) || > >>> boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV) || > >>> - opt_bhb_entry_pv || > >>> + opt_bhb_entry_pv || opt_asi_pv || > >>> opt_eager_fpu || opt_verw_pv) ? "" > >>> : " None", > >>> boot_cpu_has(X86_FEATURE_SC_MSR_PV) ? " MSR_SPEC_CTRL" > >>> : "", > >>> boot_cpu_has(X86_FEATURE_SC_RSB_PV) ? " RSB" > >>> : "", > >>> opt_eager_fpu ? " EAGER_FPU" > >>> : "", > >>> opt_verw_pv ? " VERW" > >>> : "", > >>> boot_cpu_has(X86_FEATURE_IBPB_ENTRY_PV) ? " IBPB-entry" > >>> : "", > >>> - opt_bhb_entry_pv ? " BHB-entry" > >>> : ""); > >>> + opt_bhb_entry_pv ? " BHB-entry" > >>> : "", > >>> + opt_asi_pv ? " ASI" > >>> : ""); > >>> > >>> printk(" XPTI (64-bit PV only): Dom0 %s, DomU %s (with%s PCID)\n", > >>> opt_xpti_hwdom ? "enabled" : "disabled", > >> > >> Should this printk() perhaps be suppressed when ASI is in use? > > > > Maybe, I found it useful during development to ensure the logic was > > correct, but I guess it's not of much use for plain users. I will > > make the printing conditional to ASI not being uniformly enabled. > > > > Maybe it would be useful to unify XPTI printing with the rest of > > mitigations listed in the "Support for PV VMs:" line? Albeit that > > would drop the signaling of opt_xpti_hwdom. > > Which is why I wouldn't want to "unify" it. Right I will avoid printing the line if ASI is uniformly enabled. Thanks, Roger.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |