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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <46d610e5-3f4f-4113-ac56-3ecdc66e4029@amd.com>
Date: Thu, 5 Sep 2024 17:10:47 +0530
From: Ravi Bangoria <ravi.bangoria@....com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, pbonzini@...hat.com, thomas.lendacky@....com,
 jmattson@...gle.com, hpa@...or.com, rmk+kernel@...linux.org.uk,
 peterz@...radead.org, james.morse@....com, lukas.bulwahn@...il.com,
 arjan@...ux.intel.com, j.granados@...sung.com, sibs@...natelecom.cn,
 nik.borisov@...e.com, michael.roth@....com, nikunj.dadhania@....com,
 babu.moger@....com, x86@...nel.org, kvm@...r.kernel.org,
 linux-kernel@...r.kernel.org, santosh.shukla@....com,
 ananth.narayan@....com, sandipan.das@....com, manali.shukla@....com,
 Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [PATCH v4 4/4] KVM: SVM: Add Bus Lock Detect support

On 20-Aug-24 10:08 PM, Ravi Bangoria wrote:
> Sean,
> 
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index e1b6a16e97c0..9f3d31a5d231 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -1047,7 +1047,8 @@ void svm_update_lbrv(struct kvm_vcpu *vcpu)
>>>  {
>>>  	struct vcpu_svm *svm = to_svm(vcpu);
>>>  	bool current_enable_lbrv = svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK;
>>> -	bool enable_lbrv = (svm_get_lbr_vmcb(svm)->save.dbgctl & DEBUGCTLMSR_LBR) ||
>>> +	u64 dbgctl_buslock_lbr = DEBUGCTLMSR_BUS_LOCK_DETECT | DEBUGCTLMSR_LBR;
>>> +	bool enable_lbrv = (svm_get_lbr_vmcb(svm)->save.dbgctl & dbgctl_buslock_lbr) ||
>>>  			    (is_guest_mode(vcpu) && guest_can_use(vcpu, X86_FEATURE_LBRV) &&
>>>  			    (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
>>
>> Out of sight, but this leads to calling svm_enable_lbrv() even when the guest
>> just wants to enable BUS_LOCK_DETECT.  Ignoring SEV-ES guests, KVM will intercept
>> writes to DEBUGCTL, so can't KVM defer mucking with the intercepts and
>> svm_copy_lbrs() until the guest actually wants to use LBRs?
>>
>> Hmm, and I think the existing code is broken.  If L1 passes DEBUGCTL through to
>> L2, then KVM will handles writes to L1's effective value.  And if L1 also passes
>> through the LBRs, then KVM will fail to update the MSR bitmaps for vmcb02.
>>
>> Ah, it's just a performance issue though, because KVM will still emulate RDMSR.
>>
>> Ugh, this code is silly.  The LBR MSRs are read-only, yet KVM passes them through
>> for write.
>>
>> Anyways, I'm thinking something like this?  Note, using msr_write_intercepted()
>> is wrong, because that'll check L2's bitmap if is_guest_mode(), and the idea is
>> to use L1's bitmap as the canary.
>>
>> static void svm_update_passthrough_lbrs(struct kvm_vcpu *vcpu, bool passthrough)
>> {
>> 	struct vcpu_svm *svm = to_svm(vcpu);
>>
>> 	KVM_BUG_ON(!passthrough && sev_es_guest(vcpu->kvm), vcpu->kvm);
>>
>> 	if (!msr_write_intercepted(vcpu, MSR_IA32_LASTBRANCHFROMIP) == passthrough)
>> 		return;
>>
>> 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTBRANCHFROMIP, passthrough, 0);
>> 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTBRANCHTOIP, passthrough, 0);
>> 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTINTFROMIP, passthrough, 0);
>> 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTINTTOIP, passthrough, 0);
>>
>> 	/*
>> 	 * When enabling, move the LBR msrs to vmcb02 so that L2 can see them,
>> 	 * and then move them back to vmcb01 when disabling to avoid copying
>> 	 * them on nested guest entries.
>> 	 */
>> 	if (is_guest_mode(vcpu)) {
>> 		if (passthrough)
>> 			svm_copy_lbrs(svm->vmcb, svm->vmcb01.ptr);
>> 		else
>> 			svm_copy_lbrs(svm->vmcb01.ptr, svm->vmcb);
>> 	}
>> }
>>
>> void svm_enable_lbrv(struct kvm_vcpu *vcpu)
>> {
>> 	struct vcpu_svm *svm = to_svm(vcpu);
>>
>> 	if (WARN_ON_ONCE(!sev_es_guest(vcpu->kvm)))
>> 		return;
>>
>> 	svm->vmcb->control.virt_ext |= LBR_CTL_ENABLE_MASK;
>> 	svm_update_passthrough_lbrs(vcpu, true);
>>
>> 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_DEBUGCTLMSR, 1, 1);
>> }
>>
>> static struct vmcb *svm_get_lbr_vmcb(struct vcpu_svm *svm)
>> {
>> 	/*
>> 	 * If LBR virtualization is disabled, the LBR MSRs are always kept in
>> 	 * vmcb01.  If LBR virtualization is enabled and L1 is running VMs of
>> 	 * its own, the MSRs are moved between vmcb01 and vmcb02 as needed.
>> 	 */
>> 	return svm->vmcb->control.virt_ext & LBR_CTL_ENABLE_MASK ? svm->vmcb :
>> 								   svm->vmcb01.ptr;
>> }
>>
>> void svm_update_lbrv(struct kvm_vcpu *vcpu)
>> {
>> 	struct vcpu_svm *svm = to_svm(vcpu);
>> 	u64 guest_debugctl = svm_get_lbr_vmcb(svm)->save.dbgctl;
>> 	bool enable_lbrv = (guest_debugctl & DEBUGCTLMSR_LBR) ||
>> 			    (is_guest_mode(vcpu) && guest_can_use(vcpu, X86_FEATURE_LBRV) &&
>> 			    (svm->nested.ctl.virt_ext & LBR_CTL_ENABLE_MASK));
>>
>> 	if (enable_lbrv || (guest_debugctl & DEBUGCTLMSR_BUS_LOCK_DETECT))
>> 		svm->vmcb->control.virt_ext |= LBR_CTL_ENABLE_MASK;
>> 	else
>> 		svm->vmcb->control.virt_ext &= ~LBR_CTL_ENABLE_MASK;
>>
>> 	svm_update_passthrough_lbrs(vcpu, enable_lbrv);
>> }
> 
> This refactored code looks fine. I did some sanity testing with SVM/SEV/SEV-ES
> guests and not seeing any issues. I'll respin with above change included.

Realised that KUT LBR tests were failing with this change and I had
to do this to fix those:

---
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 0b807099cb19..3dd737db85ef 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -795,6 +795,21 @@ static bool valid_msr_intercept(u32 index)
 	return direct_access_msr_slot(index) != -ENOENT;
 }
 
+static bool msr_read_intercepted_msrpm(u32 *msrpm, u32 msr)
+{
+	unsigned long tmp;
+	u8 bit_read;
+	u32 offset;
+
+	offset = svm_msrpm_offset(msr);
+	bit_read = 2 * (msr & 0x0f);
+	tmp = msrpm[offset];
+
+	BUG_ON(offset == MSR_INVALID);
+
+	return test_bit(bit_read, &tmp);
+}
+
 static bool msr_write_intercepted(struct kvm_vcpu *vcpu, u32 msr)
 {
 	u8 bit_write;
@@ -1000,7 +1015,7 @@ static void svm_update_passthrough_lbrs(struct kvm_vcpu *vcpu, bool passthrough)
 
 	KVM_BUG_ON(!passthrough && sev_es_guest(vcpu->kvm), vcpu->kvm);
 
-	if (!msr_write_intercepted(vcpu, MSR_IA32_LASTBRANCHFROMIP) == passthrough)
+	if (!msr_read_intercepted_msrpm(svm->msrpm, MSR_IA32_LASTBRANCHFROMIP) == passthrough)
 		return;
 
 	set_msr_interception(vcpu, svm->msrpm, MSR_IA32_LASTBRANCHFROMIP, passthrough, 0);
---

I've added a new api for read interception since LBR register writes are
always intercepted.

Does this looks good?

Thanks,
Ravi

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ