| Message ID | 20240726152206.28411-13-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: adventures in Address Space Isolation | expand |
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. > @@ -2414,10 +2414,10 @@ in place for guests to use. > > Use of a positive boolean value for either of these options is invalid. > > -The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=` and `bhb-entry=` > -options offer fine grained control over the primitives by Xen. These impact > -Xen's ability to protect itself, and/or Xen's ability to virtualise support > -for guests to use. > +The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=`, `bhb-entry=` and > +`asi=` options offer fine grained control over the primitives by Xen. These Here, ahead of "by Xen", it looks like "used" was missing. Maybe a good opportunity to add it? > @@ -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? > @@ -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. > @@ -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 ... > + { > + printk(XENLOG_ERR > + "XPTI is incompatible with Address Space Isolation - disabling ASI\n"); > + opt_asi_pv = 0; > + } > if ( (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) || > cpu_has_rdcl_no ) > { > @@ -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. > @@ -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? Jan
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. 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? It might end up needing slightly different processing for arches different than x86, as for x86 it should be possible to enable the option only for PV or HVM domains, while for other arches this might make no sense for not having PV support. > > @@ -2414,10 +2414,10 @@ in place for guests to use. > > > > Use of a positive boolean value for either of these options is invalid. > > > > -The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=` and `bhb-entry=` > > -options offer fine grained control over the primitives by Xen. These impact > > -Xen's ability to protect itself, and/or Xen's ability to virtualise support > > -for guests to use. > > +The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=`, `bhb-entry=` and > > +`asi=` options offer fine grained control over the primitives by Xen. These > > Here, ahead of "by Xen", it looks like "used" was missing. Maybe a good > opportunity to add it? Oh, yes. > > @@ -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? The key point is that we would only map sensitive information transiently IMO. > > @@ -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" OTOH, there's no "virtualisation support in place for guests to use" when no-xen is used. I have to admin the description for that option is not obviously clear to me, so > > @@ -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`. > > + { > > + printk(XENLOG_ERR > > + "XPTI is incompatible with Address Space Isolation - disabling ASI\n"); > > + opt_asi_pv = 0; > > + } > > if ( (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) || > > cpu_has_rdcl_no ) > > { > > @@ -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. I think the most common case is either having ASI enabled everywhere, or having ASI enabled only for domUs and the speculation mitigations also disabled for the hw domain. > > @@ -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. Thanks, Roger.
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
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.
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
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 98a45211556b..0ddc330428d9 100644 --- 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>, > bti-thunk=retpoline|lfence|jmp,bhb-seq=short|tsx|long, > {ibrs,ibpb,ssbd,psfd, > eager-fpu,l1d-flush,branch-harden,srb-lock, @@ -2414,10 +2414,10 @@ in place for guests to use. Use of a positive boolean value for either of these options is invalid. -The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=` and `bhb-entry=` -options offer fine grained control over the primitives by Xen. These impact -Xen's ability to protect itself, and/or Xen's ability to virtualise support -for guests to use. +The `pv=`, `hvm=`, `msr-sc=`, `rsb=`, `verw=`, `ibpb-entry=`, `bhb-entry=` and +`asi=` options offer fine grained control over the primitives by Xen. These +impact Xen's ability to protect itself, and/or Xen's ability to virtualise +support for guests to use. * `pv=` and `hvm=` offer control over all suboptions for PV and HVM guests respectively. @@ -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. If Xen was compiled with `CONFIG_INDIRECT_THUNK` support, `bti-thunk=` can be used to select which of the thunks gets patched into the diff --git a/xen/arch/x86/include/asm/domain.h b/xen/arch/x86/include/asm/domain.h index 9dd2e047f4de..8c366be8c75f 100644 --- 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; + /* Emulated devices enabled bitmap. */ uint32_t emulation_flags; } __cacheline_aligned; diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h index 72347ef2b959..39963c004312 100644 --- a/xen/arch/x86/include/asm/spec_ctrl.h +++ b/xen/arch/x86/include/asm/spec_ctrl.h @@ -88,6 +88,8 @@ extern uint8_t default_scf; extern int8_t opt_xpti_hwdom, opt_xpti_domu; +extern int8_t opt_asi_pv, opt_asi_hwdom, opt_asi_hvm; + extern bool cpu_has_bug_l1tf; extern int8_t opt_pv_l1tf_hwdom, opt_pv_l1tf_domu; diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index 5dc7a17b9354..2e403aad791c 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -84,6 +84,11 @@ static bool __ro_after_init opt_verw_mmio; static int8_t __initdata opt_gds_mit = -1; static int8_t __initdata opt_div_scrub = -1; +/* Address Space Isolation for PV/HVM. */ +int8_t __ro_after_init opt_asi_pv = -1; +int8_t __ro_after_init opt_asi_hwdom = -1; +int8_t __ro_after_init opt_asi_hvm = -1; + static int __init cf_check parse_spec_ctrl(const char *s) { const char *ss; @@ -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; @@ -162,6 +171,7 @@ static int __init cf_check parse_spec_ctrl(const char *s) opt_verw_pv = val; opt_ibpb_entry_pv = val; opt_bhb_entry_pv = val; + opt_asi_pv = val; } else if ( (val = parse_boolean("hvm", s, ss)) >= 0 ) { @@ -170,6 +180,7 @@ static int __init cf_check parse_spec_ctrl(const char *s) opt_verw_hvm = val; opt_ibpb_entry_hvm = val; opt_bhb_entry_hvm = val; + opt_asi_hvm = val; } else if ( (val = parse_boolean("msr-sc", s, ss)) != -1 ) { @@ -279,6 +290,27 @@ static int __init cf_check parse_spec_ctrl(const char *s) break; } } + else if ( (val = parse_boolean("asi", s, ss)) != -1 ) + { + switch ( val ) + { + case 0: + case 1: + opt_asi_pv = opt_asi_hwdom = opt_asi_hvm = val; + break; + + case -2: + s += strlen("asi="); + if ( (val = parse_boolean("pv", s, ss)) >= 0 ) + opt_asi_pv = val; + else if ( (val = parse_boolean("hvm", s, ss)) >= 0 ) + opt_asi_hvm = val; + else + default: + rc = -EINVAL; + break; + } + } /* Xen's speculative sidechannel mitigation settings. */ else if ( !strncmp(s, "bti-thunk=", 10) ) @@ -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 ) + { + printk(XENLOG_ERR + "XPTI is incompatible with Address Space Isolation - disabling ASI\n"); + opt_asi_pv = 0; + } if ( (boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON)) || cpu_has_rdcl_no ) { @@ -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; } } @@ -630,12 +669,13 @@ static void __init print_details(enum ind_thunk thunk) * mitigation support for guests. */ #ifdef CONFIG_HVM - printk(" Support for HVM VMs:%s%s%s%s%s%s%s%s\n", + printk(" Support for HVM VMs:%s%s%s%s%s%s%s%s%s\n", (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) || boot_cpu_has(X86_FEATURE_SC_RSB_HVM) || boot_cpu_has(X86_FEATURE_IBPB_ENTRY_HVM) || opt_bhb_entry_hvm || amd_virt_spec_ctrl || - opt_eager_fpu || opt_verw_hvm) ? "" : " None", + opt_eager_fpu || opt_verw_hvm || + opt_asi_hvm) ? "" : " None", boot_cpu_has(X86_FEATURE_SC_MSR_HVM) ? " MSR_SPEC_CTRL" : "", (boot_cpu_has(X86_FEATURE_SC_MSR_HVM) || amd_virt_spec_ctrl) ? " MSR_VIRT_SPEC_CTRL" : "", @@ -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", @@ -1773,6 +1815,9 @@ void spec_ctrl_init_domain(struct domain *d) if ( pv ) d->arch.pv.xpti = is_hardware_domain(d) ? opt_xpti_hwdom : opt_xpti_domu; + + d->arch.asi = is_hardware_domain(d) ? opt_asi_hwdom + : pv ? opt_asi_pv : opt_asi_hvm; } void __init init_speculation_mitigations(void) @@ -2069,6 +2114,19 @@ void __init init_speculation_mitigations(void) hw_smt_enabled && default_xen_spec_ctrl ) setup_force_cpu_cap(X86_FEATURE_SC_MSR_IDLE); + /* Disable ASI by default until feature is finished. */ + if ( opt_asi_pv == -1 ) + opt_asi_pv = 0; + if ( opt_asi_hwdom == -1 ) + opt_asi_hwdom = 0; + if ( opt_asi_hvm == -1 ) + opt_asi_hvm = 0; + + if ( opt_asi_pv || opt_asi_hvm ) + warning_add( + "Address Space Isolation is not functional, this option is\n" + "intended to be used only for development purposes.\n"); + xpti_init_default(); l1tf_calculations();
No functional change, as the option is not used. Introduced new so newly added functionality is keyed on the option being enabled, even if the feature is non-functional. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- docs/misc/xen-command-line.pandoc | 15 ++++-- xen/arch/x86/include/asm/domain.h | 3 ++ xen/arch/x86/include/asm/spec_ctrl.h | 2 + xen/arch/x86/spec_ctrl.c | 74 +++++++++++++++++++++++++--- 4 files changed, 81 insertions(+), 13 deletions(-)