lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Wed, 4 Mar 2020 09:49:14 +0800 From: Xiaoyao Li <xiaoyao.li@...el.com> To: Sean Christopherson <sean.j.christopherson@...el.com> Cc: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, hpa@...or.com, Paolo Bonzini <pbonzini@...hat.com>, Andy Lutomirski <luto@...nel.org>, tony.luck@...el.com, peterz@...radead.org, fenghua.yu@...el.com, x86@...nel.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v3 2/8] x86/split_lock: Ensure X86_FEATURE_SPLIT_LOCK_DETECT means the existence of feature On 3/4/2020 3:41 AM, Sean Christopherson wrote: > On Tue, Mar 03, 2020 at 10:55:24AM -0800, Sean Christopherson wrote: >> On Thu, Feb 06, 2020 at 03:04:06PM +0800, Xiaoyao Li wrote: >>> When flag X86_FEATURE_SPLIT_LOCK_DETECT is set, it should ensure the >>> existence of MSR_TEST_CTRL and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit. >> >> The changelog confused me a bit. "When flag X86_FEATURE_SPLIT_LOCK_DETECT >> is set" makes it sound like the logic is being applied after the feature >> bit is set. Maybe something like: >> >> ``` >> Verify MSR_TEST_CTRL.SPLIT_LOCK_DETECT can be toggled via WRMSR prior to >> setting the SPLIT_LOCK_DETECT feature bit so that runtime consumers, >> e.g. KVM, don't need to worry about WRMSR failure. >> ``` >> >>> Signed-off-by: Xiaoyao Li <xiaoyao.li@...el.com> >>> --- >>> arch/x86/kernel/cpu/intel.c | 41 +++++++++++++++++++++---------------- >>> 1 file changed, 23 insertions(+), 18 deletions(-) >>> >>> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c >>> index 2b3874a96bd4..49535ed81c22 100644 >>> --- a/arch/x86/kernel/cpu/intel.c >>> +++ b/arch/x86/kernel/cpu/intel.c >>> @@ -702,7 +702,8 @@ static void init_intel(struct cpuinfo_x86 *c) >>> if (tsx_ctrl_state == TSX_CTRL_DISABLE) >>> tsx_disable(); >>> >>> - split_lock_init(); >>> + if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) >>> + split_lock_init(); >>> } >>> >>> #ifdef CONFIG_X86_32 >>> @@ -986,9 +987,26 @@ static inline bool match_option(const char *arg, int arglen, const char *opt) >>> >>> static void __init split_lock_setup(void) >>> { >>> + u64 test_ctrl_val; >>> char arg[20]; >>> int i, ret; >>> + /* >>> + * Use the "safe" versions of rdmsr/wrmsr here to ensure MSR_TEST_CTRL >>> + * and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit do exist. Because there may >>> + * be glitches in virtualization that leave a guest with an incorrect >>> + * view of real h/w capabilities. >>> + */ >>> + if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val)) >>> + return; >>> + >>> + if (wrmsrl_safe(MSR_TEST_CTRL, >>> + test_ctrl_val | MSR_TEST_CTRL_SPLIT_LOCK_DETECT)) >>> + return; >>> + >>> + if (wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val)) >>> + return;a >> >> Probing the MSR should be skipped if SLD is disabled in sld_options, i.e. >> move this code (and setup_force_cpu_cap() etc...) down below the >> match_option() logic. The above would temporarily enable SLD even if the >> admin has explicitly disabled it, e.g. makes the kernel param useless for >> turning off the feature due to bugs. > > Hmm, but this prevents KVM from exposing SLD to a guest when it's off in > the kernel, which would be a useful debug/testing scenario. > > Maybe add another SLD state to forcefully disable SLD? That way the admin > can turn of SLD in the host kernel but still allow KVM to expose it to its > guests. E.g. I don't think we need do this. IMO, this a the bug of split_lock_init(), which assume the initial value of MSR_TEST_CTRL is zero, at least bit SPLIT_LOCK of which is zero. This is problem, it's possible that BIOS has set this bit. split_lock_setup() here, is to check if the feature really exists. So probing MSR_TEST_CTRL and bit MSR_TEST_CTRL_SPLIT_LOCK_DETECT here. If there all exist, setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT) to indicate feature does exist. Only when feature exists, there is a need to parse the command line config of split_lock_detect. In split_lock_init(), we should explicitly clear MSR_TEST_CTRL_SPLIT_LOCK_DETECT bit if sld_off. > static const struct { > const char *option; > enum split_lock_detect_state state; > } sld_options[] __initconst = { > { "disable", sld_disable }, > { "off", sld_off }, > { "warn", sld_warn }, > { "fatal", sld_fatal }, > }; > > > Then the new setup() becomes: > > static void __init split_lock_setup(void) > { > u64 test_ctrl_val; > char arg[20]; > int i, ret; > > sld_state = sld_warn; > > ret = cmdline_find_option(boot_command_line, "split_lock_detect", > arg, sizeof(arg)); > if (ret >= 0) { > for (i = 0; i < ARRAY_SIZE(sld_options); i++) { > if (match_option(arg, ret, sld_options[i].option)) { > sld_state = sld_options[i].state; > break; > } > } > } > > if (sld_state == sld_disable) > goto log_sld; > > /* > * Use the "safe" versions of rdmsr/wrmsr here to ensure MSR_TEST_CTRL > * and MSR_TEST_CTRL.SPLIT_LOCK_DETECT bit do exist. Because there may > * be glitches in virtualization that leave a guest with an incorrect > * view of real h/w capabilities. > */ > if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val)) > goto sld_broken; > > if (wrmsrl_safe(MSR_TEST_CTRL, > test_ctrl_val | MSR_TEST_CTRL_SPLIT_LOCK_DETECT)) > goto sld_broken; > > if (wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val)) > goto sld_broken; > > setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT); > > log_sld: > switch (sld_state) { > case sld_disable: > pr_info("split_lock detection disabled\n"); > break; > case sld_off: > pr_info("split_lock detection off in kernel\n"); > break; > case sld_warn: > pr_info("warning about user-space split_locks\n"); > break; > case sld_fatal: > pr_info("sending SIGBUS on user-space split_locks\n"); > break; > } > > return; > > sld_broken: > sld_state = sld_disable; > pr_err("split_lock detection disabled, MSR access faulted\n"); > } > >> And with that, IMO failing any of RDMSR/WRSMR here warrants a pr_err(). >> The CPU says it supports split lock and the admin hasn't explicitly turned >> it off, so failure to enable should be logged. >> >>> + >>> setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT); >>> sld_state = sld_warn; >>> >>> @@ -1022,24 +1040,19 @@ static void __init split_lock_setup(void) >>> * Locking is not required at the moment because only bit 29 of this >>> * MSR is implemented and locking would not prevent that the operation >>> * of one thread is immediately undone by the sibling thread. >>> - * Use the "safe" versions of rdmsr/wrmsr here because although code >>> - * checks CPUID and MSR bits to make sure the TEST_CTRL MSR should >>> - * exist, there may be glitches in virtualization that leave a guest >>> - * with an incorrect view of real h/w capabilities. >>> */ >>> -static bool __sld_msr_set(bool on) >>> +static void __sld_msr_set(bool on) >>> { >>> u64 test_ctrl_val; >>> >>> - if (rdmsrl_safe(MSR_TEST_CTRL, &test_ctrl_val)) >>> - return false; >>> + rdmsrl(MSR_TEST_CTRL, test_ctrl_val); >>> >>> if (on) >>> test_ctrl_val |= MSR_TEST_CTRL_SPLIT_LOCK_DETECT; >>> else >>> test_ctrl_val &= ~MSR_TEST_CTRL_SPLIT_LOCK_DETECT; >>> >>> - return !wrmsrl_safe(MSR_TEST_CTRL, test_ctrl_val); >>> + wrmsrl(MSR_TEST_CTRL, test_ctrl_val); >>> } >>> >>> static void split_lock_init(void) >>> @@ -1047,15 +1060,7 @@ static void split_lock_init(void) >>> if (sld_state == sld_off) >>> return; >>> >>> - if (__sld_msr_set(true)) >>> - return; >>> - >>> - /* >>> - * If this is anything other than the boot-cpu, you've done >>> - * funny things and you get to keep whatever pieces. >>> - */ >>> - pr_warn("MSR fail -- disabled\n"); >>> - sld_state = sld_off; >>> + __sld_msr_set(true); >>> } >>> >>> bool handle_user_split_lock(unsigned long ip) >>> -- >>> 2.23.0 >>>
Powered by blists - more mailing lists