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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

>>> @@ -2449,6 +2449,11 @@ for guests to use.
>>>    is not available (see `bhi-dis-s`).  The choice of scrubbing sequence 
>>> can be
>>>    selected using the `bhb-seq=` option.  If it is necessary to protect dom0
>>>    too, boot with `spec-ctrl=bhb-entry`.
>>> +* `asi=` offers control over whether the hypervisor will engage in Address
>>> +  Space Isolation, by not having sensitive information mapped in the VMM
>>> +  page-tables.  Not having sensitive information on the page-tables avoids
>>> +  having to perform some mitigations for speculative attacks when
>>> +  context-switching to the hypervisor.
>>
>> Is "not having" and ...
>>
>>> --- a/xen/arch/x86/include/asm/domain.h
>>> +++ b/xen/arch/x86/include/asm/domain.h
>>> @@ -458,6 +458,9 @@ struct arch_domain
>>>      /* Don't unconditionally inject #GP for unhandled MSRs. */
>>>      bool msr_relaxed;
>>>  
>>> +    /* Run the guest without sensitive information in the VMM page-tables. 
>>> */
>>> +    bool asi;
>>
>> ... "without" really going to be fully true? Wouldn't we better say "as 
>> little
>> as possible" or alike?
> 
> Maybe better use:
> 
> "...by not having sensitive information permanently mapped..."
> 
> And a similar adjustment to the comment?

Yes, that's better.

>>> @@ -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.

>>> @@ -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.

>>> @@ -389,9 +428,9 @@ static __init void xpti_init_default(void)
>>>      else
>>>      {
>>>          if ( opt_xpti_hwdom < 0 )
>>> -            opt_xpti_hwdom = 1;
>>> +            opt_xpti_hwdom = !opt_asi_hwdom;
>>>          if ( opt_xpti_domu < 0 )
>>> -            opt_xpti_domu = 1;
>>> +            opt_xpti_domu = !opt_asi_pv;
>>>      }
>>
>> ... here?
>>
>> It would further seem desirable to me if opt_asi_hwdom had its default set
>> later, when we know the kind of Dom0, such that it could be defaulted to
>> what opt_asi_{hvm,pv} are set to. This, however, wouldn't be compatible
>> with the use here. Perhaps the invocation of xpti_init_default() would
>> need deferring, too.
> 
> Given the current parsing logic, opt_asi_hwdom will only be set when
> both opt_asi_{hvm,pv} are set.  Setting spec-ctrl=asi={pv,hvm} will
> only enable ASI for the domUs of the selected mode.
> 
> Hence deferring won't make any practical difference, as having
> opt_asi_hwdom enabled implies having ASI enabled for all domain
> types.

Right, another effect of me not having paid enough attention to that parsing
detail.

>>> @@ -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.

Jan