|
[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 |