linux-kernel - Re: [BUG] arch/x86/kvm/vmx/vmx_onhyperv.h:109:36: error: dereference of NULL ‘0’

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] [day] [month] [year] [list]

Message-ID: <Zr4GKEzp8eQDDH1d@google.com>
Date: Thu, 15 Aug 2024 06:44:08 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>
Cc: Mirsad Todorovac <mtodorovac69@...il.com>, kvm@...r.kernel.org, 
	Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [BUG] arch/x86/kvm/vmx/vmx_onhyperv.h:109:36: error: dereference of NULL ‘0’

On Thu, Aug 15, 2024, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@...gle.com> writes:
> 
> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> >> Sean Christopherson <seanjc@...gle.com> writes:
> >> 
> >> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> >> >> What I meant is something along these lines (untested):
> >> >> 
> >> >> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
> >> >> index eb48153bfd73..e2d8c67d0cad 100644
> >> >> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h
> >> >> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
> >> >> @@ -104,6 +104,14 @@ static inline void evmcs_load(u64 phys_addr)
> >> >>         struct hv_vp_assist_page *vp_ap =
> >> >>                 hv_get_vp_assist_page(smp_processor_id());
> >> >>  
> >> >> +       /*
> >> >> +        * When enabling eVMCS, KVM verifies that every CPU has a valid hv_vp_assist_page()
> >> >> +        * and aborts enabling the feature otherwise. CPU onlining path is also checked in
> >> >> +        * vmx_hardware_enable(). With this, it is impossible to reach here with vp_ap == NULL
> >> >> +        * but compilers may still complain.
> >> >> +        */
> >> >> +       BUG_ON(!vp_ap);
> >> >
> >> > A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
> >> > check here and do more than just WARN, then it's easy enough to plumb in @vcpu
> >> > and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
> >> > corrupting the guest somehow.
> >> >
> >> 
> >> I'm still acting under the impression this is an absolutely impossible
> >> situation :-)
> >> 
> >> AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't
> >> have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the
> >> plumbing (or am I missing an easy way to go back from @vmcs to
> >> @vcpu?). On the other hand, vmcs_load() should not be called that ofter
> >> so if we prefer to have @vcpu there for some other reason -- why not.
> >
> > kvm_get_running_vcpu(), though I honestly purposely didn't suggest it earlier
> > because I am not a fan of using kvm_get_running_vcpu() unless it's absolutely
> > necessary.  But for this situation, I'd be fine with using it.
> 
> Ah, nice, so we don't even need the plumbing then I guess? Compile-tested only:
> 
> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
> index eb48153bfd73..318f5f95f211 100644
> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h
> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
> @@ -104,6 +104,19 @@ static inline void evmcs_load(u64 phys_addr)
>         struct hv_vp_assist_page *vp_ap =
>                 hv_get_vp_assist_page(smp_processor_id());
>  
> +       /*
> +        * When enabling eVMCS, KVM verifies that every CPU has a valid hv_vp_assist_page()
> +        * and aborts enabling the feature otherwise. CPU onlining path is also checked in
> +        * vmx_hardware_enable(). With this, it is impossible to reach here with vp_ap == NULL
> +        * but compilers may still complain.
> +        */
> +       if (!vp_ap) {
> +               struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> +
> +               KVM_BUG_ON(1, vcpu->kvm);
> +               return;

Eh, I would just do:

	if (KVM_BUG_ON(!vp_ap, kvm_get_running_vcpu()->kvm))
		return

> +       }
> +
>         if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
>                 vp_ap->nested_control.features.directhypercall = 1;
>         vp_ap->current_nested_vmcs = phys_addr;
> 
> (I hope we can't reach here with kvm_running_vcpu unset, can we?)

Yes?  kvm_running_vcpu is set before kvm_arch_vcpu_load() and cleared after
kvm_arch_vcpu_put(), and I can't think of a scenario where it would be legal/sane
to invoke vmcs_load() without a running/loaded vCPU.  VMX needs the current VMCS
to be loaded to ensure guest state can be accessed, so any ioctl() that can touch
guest state needs to do vcpu_load().

x86's kvm_arch_vcpu_ioctl() unconditionally does vcpu_load(), and the only ioctls
I see in kvm_vcpu_ioctl() that _don't_ do vcpu_load() are KVM_SET_SIGNAL_MASK and
KVM_GET_STATS_FD, so I think we're good.

And if I'm wrong and the impossible happens twice, so be it, we die on #GP :-)