| 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
| ||
|
Message-ID: <20240325231058.GP2357401@ls.amr.corp.intel.com>
Date: Mon, 25 Mar 2024 16:10:58 -0700
From: Isaku Yamahata <isaku.yamahata@...el.com>
To: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
Cc: "Yamahata, Isaku" <isaku.yamahata@...el.com>,
"Zhang, Tina" <tina.zhang@...el.com>,
"seanjc@...gle.com" <seanjc@...gle.com>,
"Huang, Kai" <kai.huang@...el.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>,
"Chen, Bo2" <chen.bo@...el.com>,
"sagis@...gle.com" <sagis@...gle.com>,
"isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Aktas, Erdem" <erdemaktas@...gle.com>,
"isaku.yamahata@...ux.intel.com" <isaku.yamahata@...ux.intel.com>,
"sean.j.christopherson@...el.com" <sean.j.christopherson@...el.com>,
"Yuan, Hang" <hang.yuan@...el.com>,
"kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: Re: [PATCH v19 059/130] KVM: x86/tdp_mmu: Don't zap private pages
for unsupported cases
On Mon, Mar 25, 2024 at 03:18:36PM -0700,
Isaku Yamahata <isaku.yamahata@...el.com> wrote:
> On Mon, Mar 25, 2024 at 07:55:04PM +0000,
> "Edgecombe, Rick P" <rick.p.edgecombe@...el.com> wrote:
>
> > On Mon, 2024-03-25 at 12:05 -0700, Isaku Yamahata wrote:
> > > Right, the guest has to accept it on VE. If the unmap was intentional by guest,
> > > that's fine. The unmap is unintentional (with vMTRR), the guest doesn't expect
> > > VE with the GPA.
> > >
> > >
> > > > But, I guess we should punt to userspace is the guest tries to use
> > > > MTRRs, not that userspace can handle it happening in a TD... But it
> > > > seems cleaner and safer then skipping zapping some pages inside the
> > > > zapping code.
> > > >
> > > > I'm still not sure if I understand the intention and constraints fully.
> > > > So please correct. This (the skipping the zapping for some operations)
> > > > is a theoretical correctness issue right? It doesn't resolve a TD
> > > > crash?
> > >
> > > For lapic, it's safe guard. Because TDX KVM disables APICv with
> > > APICV_INHIBIT_REASON_TDX, apicv won't call kvm_zap_gfn_range().
> > Ah, I see it:
> > https://lore.kernel.org/lkml/38e2f8a77e89301534d82325946eb74db3e47815.1708933498.git.isaku.yamahata@intel.com/
> >
> > Then it seems a warning would be more appropriate if we are worried there might be a way to still
> > call it. If we are confident it can't, then we can just ignore this case.
> >
> > >
> > > For MTRR, the purpose is to make the guest boot (without the guest kernel
> > > command line like clearcpuid=mtrr) .
> > > If we can assume the guest won't touch MTRR registers somehow, KVM can return an
> > > error to TDG.VP.VMCALL<RDMSR, WRMSR>(MTRR registers). So it doesn't call
> > > kvm_zap_gfn_range(). Or we can use KVM_EXIT_X86_{RDMSR, WRMSR} as you suggested.
> >
> > My understanding is that Sean prefers to exit to userspace when KVM can't handle something, versus
> > making up behavior that keeps known guests alive. So I would think we should change this patch to
> > only be about not using the zapping roots optimization. Then a separate patch should exit to
> > userspace on attempt to use MTRRs. And we ignore the APIC one.
> >
> > This is trying to guess what maintainers would want here. I'm less sure what Paolo prefers.
>
> When we hit KVM_MSR_FILTER, the current implementation ignores it and makes it
> error to guest. Surely we should make it KVM_EXIT_X86_{RDMSR, WRMSR}, instead.
> It's aligns with the existing implementation(default VM and SW-protected) and
> more flexible.
Something like this for "112/130 KVM: TDX: Handle TDX PV rdmsr/wrmsr hypercall"
Compile only tested at this point.
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index f891de30a2dd..4d9ae5743e24 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1388,31 +1388,67 @@ static int tdx_emulate_mmio(struct kvm_vcpu *vcpu)
return 1;
}
+static int tdx_complete_rdmsr(struct kvm_vcpu *vcpu)
+{
+ if (vcpu->run->msr.error)
+ tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
+ else {
+ tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
+ tdvmcall_set_return_val(vcpu, vcpu->run->msr.data);
+ }
+ return 1;
+}
+
static int tdx_emulate_rdmsr(struct kvm_vcpu *vcpu)
{
u32 index = tdvmcall_a0_read(vcpu);
u64 data;
- if (!kvm_msr_allowed(vcpu, index, KVM_MSR_FILTER_READ) ||
- kvm_get_msr(vcpu, index, &data)) {
+ if (!kvm_msr_allowed(vcpu, index, KVM_MSR_FILTER_READ)) {
+ trace_kvm_msr_read_ex(index);
+ tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
+ return kvm_msr_user_space(vcpu, index, KVM_EXIT_X86_RDMSR, 0,
+ tdx_complete_rdmsr,
+ KVM_MSR_RET_FILTERED);
+ }
+
+ if (kvm_get_msr(vcpu, index, &data)) {
trace_kvm_msr_read_ex(index);
tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
return 1;
}
- trace_kvm_msr_read(index, data);
+ trace_kvm_msr_read(index, data);
tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
tdvmcall_set_return_val(vcpu, data);
return 1;
}
+static int tdx_complete_wrmsr(struct kvm_vcpu *vcpu)
+{
+ if (vcpu->run->msr.error)
+ tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
+ else
+ tdvmcall_set_return_code(vcpu, TDVMCALL_SUCCESS);
+ return 1;
+}
+
static int tdx_emulate_wrmsr(struct kvm_vcpu *vcpu)
{
u32 index = tdvmcall_a0_read(vcpu);
u64 data = tdvmcall_a1_read(vcpu);
- if (!kvm_msr_allowed(vcpu, index, KVM_MSR_FILTER_WRITE) ||
- kvm_set_msr(vcpu, index, data)) {
+ if (!kvm_msr_allowed(vcpu, index, KVM_MSR_FILTER_WRITE)) {
+ trace_kvm_msr_write_ex(index, data);
+ tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
+ if (kvm_msr_user_space(vcpu, index, KVM_EXIT_X86_WRMSR, data,
+ tdx_complete_wrmsr,
+ KVM_MSR_RET_FILTERED))
+ return 1;
+ return 0;
+ }
+
+ if (kvm_set_msr(vcpu, index, data)) {
trace_kvm_msr_write_ex(index, data);
tdvmcall_set_return_code(vcpu, TDVMCALL_INVALID_OPERAND);
return 1;
--
Isaku Yamahata <isaku.yamahata@...el.com>
Powered by blists - more mailing lists