| Message ID | 59AEC14A0200007800177861@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/09/17 14:22, Jan Beulich wrote: > For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd > need to clone the respective hack used for CPUID_FAULTING. Introduce an > inverse of setup_clear_cpu_cap() instead, but let clearing of features > overrule forced setting of them. > > XEN_SMAP being wrong post-boot is a problem specifically for live > patching, as a live patch may need alternative instruction patching > keyed off of that feature flag. > > Reported-by: Sarah Newman <security@prgmr.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 09/05/2017 06:22 AM, Jan Beulich wrote: > For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd > need to clone the respective hack used for CPUID_FAULTING. Introduce an > inverse of setup_clear_cpu_cap() instead, but let clearing of features > overrule forced setting of them. > > XEN_SMAP being wrong post-boot is a problem specifically for live > patching, as a live patch may need alternative instruction patching > keyed off of that feature flag. > > Reported-by: Sarah Newman <security@prgmr.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> Reported-by/Tested-by: Sarah Newman <srn@prgmr.com> Some questions: It looks like setup_clear_cpu_cap has a search for dependent capabilities that also must be cleared. Does the same need to happen for setup_force_cpu_cap even if it doesn't matter for the current cpu features? Does it make sense to add a comment where forced_caps is declared that cleared_caps overrides forced_caps instead of just in the commit message?
>>> Sarah Newman <srn@prgmr.com> 09/07/17 3:55 AM >>> >On 09/05/2017 06:22 AM, Jan Beulich wrote: >> For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd >> need to clone the respective hack used for CPUID_FAULTING. Introduce an >> inverse of setup_clear_cpu_cap() instead, but let clearing of features >> overrule forced setting of them. >> >> XEN_SMAP being wrong post-boot is a problem specifically for live >> patching, as a live patch may need alternative instruction patching >> keyed off of that feature flag. >> >> Reported-by: Sarah Newman <security@prgmr.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > >Reported-by/Tested-by: Sarah Newman <srn@prgmr.com> Thanks. >Some questions: > >It looks like setup_clear_cpu_cap has a search for dependent capabilities >that also must be cleared. Does the same need to happen for >setup_force_cpu_cap even if it doesn't matter for the current cpu features? We obviously can't force-set dependent capabilities, and forcing a feature on won't result in the need to clear any others (unless of course it was a strange inverse "feature", but for those it would probably be better to not force them on in the first place). >Does it make sense to add a comment where forced_caps is declared >that cleared_caps overrides forced_caps instead of just in the commit >message? From the code it should be pretty obvious, I would think. But of course you're free to submit a patch to add comments if you feel strongly about it. Jan
--- a/xen/arch/x86/apic.c +++ b/xen/arch/x86/apic.c @@ -853,7 +853,7 @@ static int __init detect_init_APIC (void return -1; } - __set_bit(X86_FEATURE_APIC, boot_cpu_data.x86_capability); + setup_force_cpu_cap(X86_FEATURE_APIC); mp_lapic_addr = APIC_DEFAULT_PHYS_BASE; /* The BIOS may have set up the APIC at some other address */ --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -54,6 +54,7 @@ unsigned int vaddr_bits __read_mostly = u64 host_pat = 0x050100070406; static unsigned int cleared_caps[NCAPINTS]; +static unsigned int forced_caps[NCAPINTS]; void __init setup_clear_cpu_cap(unsigned int cap) { @@ -63,6 +64,10 @@ void __init setup_clear_cpu_cap(unsigned if (__test_and_set_bit(cap, cleared_caps)) return; + if (test_bit(cap, forced_caps)) + printk("%pS clearing previously forced feature %#x\n", + __builtin_return_address(0), cap); + __clear_bit(cap, boot_cpu_data.x86_capability); dfs = lookup_deep_deps(cap); @@ -72,7 +77,26 @@ void __init setup_clear_cpu_cap(unsigned for (i = 0; i < FSCAPINTS; ++i) { cleared_caps[i] |= dfs[i]; boot_cpu_data.x86_capability[i] &= ~dfs[i]; + if (!(forced_caps[i] & dfs[i])) + continue; + printk("%pS implicitly clearing previously forced feature(s) %u:%#x\n", + __builtin_return_address(0), + i, forced_caps[i] & dfs[i]); + } +} + +void __init setup_force_cpu_cap(unsigned int cap) +{ + if (__test_and_set_bit(cap, forced_caps)) + return; + + if (test_bit(cap, cleared_caps)) { + printk("%pS tries to force previously cleared feature %#x\n", + __builtin_return_address(0), cap); + return; } + + __set_bit(cap, boot_cpu_data.x86_capability); } static void default_init(struct cpuinfo_x86 * c) @@ -375,8 +399,10 @@ void identify_cpu(struct cpuinfo_x86 *c) for (i = 0; i < FSCAPINTS; ++i) c->x86_capability[i] &= known_features[i]; - for (i = 0 ; i < NCAPINTS ; ++i) + for (i = 0 ; i < NCAPINTS ; ++i) { + c->x86_capability[i] |= forced_caps[i]; c->x86_capability[i] &= ~cleared_caps[i]; + } /* If the model name is still unset, do table lookup. */ if ( !c->x86_model_id[0] ) { --- a/xen/arch/x86/cpu/intel.c +++ b/xen/arch/x86/cpu/intel.c @@ -27,7 +27,7 @@ static bool __init probe_intel_cpuid_fau expected_levelling_cap |= LCAP_faulting; levelling_caps |= LCAP_faulting; - __set_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability); + setup_force_cpu_cap(X86_FEATURE_CPUID_FAULTING); return 1; } @@ -320,9 +320,6 @@ static void early_init_intel(struct cpui if (c == &boot_cpu_data) intel_init_levelling(); - if (test_bit(X86_FEATURE_CPUID_FAULTING, boot_cpu_data.x86_capability)) - __set_bit(X86_FEATURE_CPUID_FAULTING, c->x86_capability); - intel_ctxt_switch_levelling(NULL); } --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -1485,14 +1485,14 @@ void __init noreturn __start_xen(unsigne if ( !opt_smep ) setup_clear_cpu_cap(X86_FEATURE_SMEP); if ( cpu_has_smep && opt_smep != SMEP_HVM_ONLY ) - __set_bit(X86_FEATURE_XEN_SMEP, boot_cpu_data.x86_capability); + setup_force_cpu_cap(X86_FEATURE_XEN_SMEP); if ( boot_cpu_has(X86_FEATURE_XEN_SMEP) ) set_in_cr4(X86_CR4_SMEP); if ( !opt_smap ) setup_clear_cpu_cap(X86_FEATURE_SMAP); if ( cpu_has_smap && opt_smap != SMAP_HVM_ONLY ) - __set_bit(X86_FEATURE_XEN_SMAP, boot_cpu_data.x86_capability); + setup_force_cpu_cap(X86_FEATURE_XEN_SMAP); if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) ) set_in_cr4(X86_CR4_SMAP); --- a/xen/include/asm-x86/processor.h +++ b/xen/include/asm-x86/processor.h @@ -169,6 +169,7 @@ extern const struct x86_cpu_id *x86_matc extern void identify_cpu(struct cpuinfo_x86 *); extern void setup_clear_cpu_cap(unsigned int); +extern void setup_force_cpu_cap(unsigned int); extern void print_cpu_info(unsigned int cpu); extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c);
For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd need to clone the respective hack used for CPUID_FAULTING. Introduce an inverse of setup_clear_cpu_cap() instead, but let clearing of features overrule forced setting of them. XEN_SMAP being wrong post-boot is a problem specifically for live patching, as a live patch may need alternative instruction patching keyed off of that feature flag. Reported-by: Sarah Newman <security@prgmr.com> Signed-off-by: Jan Beulich <jbeulich@suse.com>