Re: [PATCH 12/22] x86/spec-ctrl: introduce Address Space Isolation command line option

["Roger Pau Monné" <roger.pau@citrix.com>]

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.