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 PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Sun, 28 Apr 2019 15:34:26 +0800 From: Xiaoyao Li <xiaoyao.li@...ux.intel.com> To: Thomas Gleixner <tglx@...utronix.de> Cc: Fenghua Yu <fenghua.yu@...el.com>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, H Peter Anvin <hpa@...or.com>, Paolo Bonzini <pbonzini@...hat.com>, Dave Hansen <dave.hansen@...el.com>, Ashok Raj <ashok.raj@...el.com>, Peter Zijlstra <peterz@...radead.org>, Ravi V Shankar <ravi.v.shankar@...el.com>, Christopherson Sean J <sean.j.christopherson@...el.com>, Kalle Valo <kvalo@...eaurora.org>, Michael Chan <michael.chan@...adcom.com>, linux-kernel <linux-kernel@...r.kernel.org>, x86 <x86@...nel.org>, kvm@...r.kernel.org, netdev@...r.kernel.org, linux-wireless@...r.kernel.org Subject: Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL On 4/28/2019 3:09 PM, Thomas Gleixner wrote: > On Sat, 27 Apr 2019, Xiaoyao Li wrote: >> On Thu, 2019-04-25 at 09:42 +0200, Thomas Gleixner wrote: >>> On Wed, 24 Apr 2019, Fenghua Yu wrote: >>>> >>>> +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx) >>>> +{ >>>> + u64 host_msr_test_ctl; >>>> + >>>> + if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) >>>> + return; >>> >>> Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask. >>> >>>> + host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache); >>>> + >>>> + if (host_msr_test_ctl == vmx->msr_test_ctl) { >>> >>> This still assumes that the only bit which can be set in the MSR is that >>> lock detect bit. >>> >>>> + clear_atomic_switch_msr(vmx, MSR_TEST_CTL); >>>> + } else { >>>> + add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl, >>>> + host_msr_test_ctl, false); >>> >>> So what happens here is that if any other bit is set on the host, VMENTER >>> will happily clear it. >> >> There are two bits of MSR TEST_CTL defined in Intel SDM now, which is bit >> 29 and bit 31. Bit 31 is not used in kernel, and here we only need to >> switch bit 29 between host and guest. So should I also change the name >> to atomic_switch_split_lock_detect() to indicate that we only switch bit >> 29? > > No. Just because we ony use the split lock bit now, there is no > jusification to name everything splitlock. This is going to have renamed > when yet another bit is added in the future. The MSR is exposed to the > guest and the restriction of bits happens to be splitlock today. Got it. >>> guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl; >>> >>> That preserves any bits which are not exposed to the guest. >>> >>> But the way more interesting question is why are you exposing the MSR and >>> the bit to the guest at all if the host has split lock detection enabled? >>> >>> That does not make any sense as you basically allow the guest to switch it >>> off and then launch a slowdown attack. If the host has it enabled, then a >>> guest has to be treated like any other process and the #AC trap has to be >>> caught by the hypervisor which then kills the guest. >>> >>> Only if the host has split lock detection disabled, then you can expose it >>> and allow the guest to turn it on and handle it on its own. >> >> Indeed, if we use split lock detection for protection purpose, when host >> has it enabled we should directly pass it to guest and forbid guest from >> disabling it. And only when host disables split lock detection, we can >> expose it and allow the guest to turn it on. > ? >> If it is used for protection purpose, then it should follow what you said and >> this feature needs to be disabled by default. Because there are split lock >> issues in old/current kernels and BIOS. That will cause the existing guest >> booting failure and killed due to those split lock. > > Rightfully so. So, the patch 13 "Enable split lock detection by default" needs to be removed? >> If it is only used for debug purpose, I think it might be OK to enable this >> feature by default and make it indepedent between host and guest? > > No. It does not make sense. > >> So I think how to handle this feature between host and guest depends on how we >> use it? Once you give me a decision, I will follow it in next version. > > As I said: The host kernel makes the decision. > > If the host kernel has it enabled then the guest is not allowed to change > it. If the guest triggers an #AC it will be killed. > > If the host kernel has it disabled then the guest can enable it for it's > own purposes. > > Thanks, > > tglx >
Powered by blists - more mailing lists