On Thu, Jun 4, 2020 at 9:43 AM Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote: > > Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes: > > > On Thu, Jun 04, 2020 at 05:33:25PM +0200, Vitaly Kuznetsov wrote: > >> Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes: > >> > >> > On Thu, Jun 04, 2020 at 04:40:52PM +0200, Paolo Bonzini wrote: > >> >> On 04/06/20 16:31, Vitaly Kuznetsov wrote: > >> > > >> > ... > >> > > >> >> > KVM could've handled the request correctly by going to userspace and > >> >> > performing I/O but there doesn't seem to be a good need for such requests > >> >> > in the first place. Sane guests should not call VMXON/VMPTRLD/VMCLEAR with > >> >> > anything but normal memory. Just inject #GP to find insane ones. > >> >> > > >> > >> ... > >> > >> >> > >> >> looks good but we need to do the same in handle_vmread, handle_vmwrite, > >> >> handle_invept and handle_invvpid. Which probably means adding something > >> >> like nested_inject_emulation_fault to commonize the inner "if". > >> > > >> > Can we just kill the guest already instead of throwing more hacks at this > >> > and hoping something sticks? We already have one in > >> > kvm_write_guest_virt_system... > >> > > >> > commit 541ab2aeb28251bf7135c7961f3a6080eebcc705 > >> > Author: Fuqian Huang <huangfq.daxian@xxxxxxxxx> > >> > Date: Thu Sep 12 12:18:17 2019 +0800 > >> > > >> > KVM: x86: work around leak of uninitialized stack contents > >> > > >> > >> Oh I see... > >> > >> [...] > >> > >> Let's get back to 'vm_bugged' idea then? > >> > >> https://lore.kernel.org/kvm/87muadnn1t.fsf@xxxxxxxxxxxxxxxxxxxx/ > > > > Hmm, I don't think we need to go that far. The 'vm_bugged' idea was more > > to handle cases where KVM itself (or hardware) screwed something up and > > detects an issue deep in a call stack with no recourse for reporting the > > error up the stack. > > > > That isn't the case here. Unless I'm mistaken, the end result is simliar > > to this patch, except that KVM would exit to userspace with > > KVM_INTERNAL_ERROR_EMULATION instead of injecting a #GP. E.g. > > I just wanted to resurrect that 'vm_bugged' idea but was waiting for a > good opportunity :-) > > The advantage of KVM_EXIT_INTERNAL_ERROR is that we're not trying to > invent some behavior which is not in SDM and making it a bit more likely > that we get a bug report from an angry user. If KVM can't handle the emulation, KVM_EXIT_INTERNAL_ERROR is far better than cooking up fictional faults to deliver to the guest. > > > > diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c > > index 9c74a732b08d..e13d2c0014e2 100644 > > --- a/arch/x86/kvm/vmx/nested.c > > +++ b/arch/x86/kvm/vmx/nested.c > > @@ -4624,6 +4624,20 @@ void nested_vmx_pmu_entry_exit_ctls_update(struct kvm_vcpu *vcpu) > > } > > } > > > > +static int nested_vmx_handle_memory_failure(struct kvm_vcpu *vcpu, int ret, > > + struct x86_exception *e) > > +{ > > + if (r == X86EMUL_PROPAGATE_FAULT) { > > + kvm_inject_emulated_page_fault(vcpu, &e); > > + return 1; > > + } > > + > > + vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR; > > + vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_EMULATION; > > + vcpu->run->internal.ndata = 0; > > + return 0; > > +} > > + > > static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer) > > { > > gva_t gva; > > @@ -4634,11 +4648,9 @@ static int nested_vmx_get_vmptr(struct kvm_vcpu *vcpu, gpa_t *vmpointer) > > sizeof(*vmpointer), &gva)) > > return 1; > > > > - if (kvm_read_guest_virt(vcpu, gva, vmpointer, sizeof(*vmpointer), &e)) { > > - kvm_inject_emulated_page_fault(vcpu, &e); > > - return 1; > > - } > > - > > + r kvm_read_guest_virt(vcpu, gva, vmpointer, sizeof(*vmpointer), &e); > > + if (r) > > + return nested_vmx_handle_memory_failure(r, &e); > > return 0; > > } > > > > ... and the same for handle_vmread, handle_vmwrite, handle_invept and > handle_invvpid as suggested by Paolo. I'll be sending this as v2 with > your Suggested-by: shortly. > > > > > > > Side topic, I have some preliminary patches for the 'vm_bugged' idea. I'll > > try to whip them into something that can be posted upstream in the next few > > weeks. > > > > Sounds great! > > -- > Vitaly >
