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

Message-ID: <Zr0rEy0bO1ju_f1C@google.com>
Date: Wed, 14 Aug 2024 15:09:23 -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 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.