linux-kernel - Re: [PATCH 10/14] KVM: x86: Make Hyper-V emulation optional

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 30 Nov 2023 16:11:43 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 10/14] KVM: x86: Make Hyper-V emulation optional

Sean Christopherson <seanjc@...gle.com> writes:

...

>
>>  static bool nested_get_vmcs12_pages(struct kvm_vcpu *vcpu)
>>  {
>> @@ -3552,11 +3563,13 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
>>  	if (!nested_vmx_check_permission(vcpu))
>>  		return 1;
>>  
>> +#ifdef CONFIG_KVM_HYPERV
>>  	evmptrld_status = nested_vmx_handle_enlightened_vmptrld(vcpu, launch);
>>  	if (evmptrld_status == EVMPTRLD_ERROR) {
>>  		kvm_queue_exception(vcpu, UD_VECTOR);
>>  		return 1;
>>  	}
>> +#endif
>>  
>>  	kvm_pmu_trigger_event(vcpu, PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
>
> This fails to build with CONFIG_KVM_HYPERV=n && CONFIG_KVM_WERROR=y:
>
> arch/x86/kvm/vmx/nested.c:3577:9: error: variable 'evmptrld_status' is uninitialized when used here [-Werror,-Wuninitialized]
>         if (CC(evmptrld_status == EVMPTRLD_VMFAIL))
>                ^~~~~~~~~~~~~~~
>
> Sadly, simply wrapping with an #ifdef also fails because then evmptrld_status
> becomes unused.  I'd really prefer to avoid having to tag it __maybe_unused, and
> adding more #ifdef would also be ugly.  Any ideas?

A couple,

- we can try putting all eVMPTR logic under 'if (1)' just to create a
  block where we can define evmptrld_status. Not sure this is nicer than
  another #ifdef wrapping evmptrld_status and I'm not sure what to do
  with kvm_pmu_trigger_event() -- can it just go above
  nested_vmx_handle_enlightened_vmptrld()?

- we can add a helper, e.g. 'evmptr_is_vmfail()' and make it just return
  'false' when !CONFIG_KVM_HYPERV.

- rewrite this as a switch to avoid the need for having the local
  variable, (untested)

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index c5ec0ef51ff7..b26ce7d596e9 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -3553,22 +3553,23 @@ static int nested_vmx_run(struct kvm_vcpu *vcpu, bool launch)
        enum nvmx_vmentry_status status;
        struct vcpu_vmx *vmx = to_vmx(vcpu);
        u32 interrupt_shadow = vmx_get_interrupt_shadow(vcpu);
-       enum nested_evmptrld_status evmptrld_status;
 
        if (!nested_vmx_check_permission(vcpu))
                return 1;
 
-       evmptrld_status = nested_vmx_handle_enlightened_vmptrld(vcpu, launch);
-       if (evmptrld_status == EVMPTRLD_ERROR) {
+       switch (nested_vmx_handle_enlightened_vmptrld(vcpu, launch)) {
+       case EVMPTRLD_ERROR:
                kvm_queue_exception(vcpu, UD_VECTOR);
                return 1;
+       case EVMPTRLD_VMFAIL:
+               trace_kvm_nested_vmenter_failed("evmptrld_status", 0);
+               return nested_vmx_failInvalid(vcpu);
+       default:
+               break;
        }
 
        kvm_pmu_trigger_event(vcpu, PERF_COUNT_HW_BRANCH_INSTRUCTIONS);
 
-       if (CC(evmptrld_status == EVMPTRLD_VMFAIL))
-               return nested_vmx_failInvalid(vcpu);
-
        if (CC(!evmptr_is_valid(vmx->nested.hv_evmcs_vmptr) &&
               vmx->nested.current_vmptr == INVALID_GPA))
                return nested_vmx_failInvalid(vcpu);

Unfortunately, I had to open code CC() ;-(

Or maybe just another "#ifdef" is not so ugly after all? :-)

-- 
Vitaly