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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.09.2024 17:27, Roger Pau Monné wrote:
> 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.

My point was really to share as much as possible, if this was a top-level
option. Of course ...

> Along the lines of:
> 
> asi= boolean | { pv, hvm, hwdom }
> 
> Or similar?
> 
> Even then sub-options would likely be different between architectures.

... the sub-options wouldn't all be generalizable.

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

I wonder whether Andrew knows of a clear way of expressing where that line
is intended to be drawn.

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

Hmm, indeed.

Jan