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

[Vitaly Kuznetsov]

Mirsad Todorovac <mtodorovac69@xxxxxxxxx> writes:

> On 7/29/24 15:31, Vitaly Kuznetsov wrote:
>> Mirsad Todorovac <mtodorovac69@xxxxxxxxx> writes:
>> 
>>> On 7/19/24 20:53, Sean Christopherson wrote:
>>>> On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
>>>>> Hi, all!
>>>>>
>>>>> Here is another potential NULL pointer dereference in kvm subsystem of linux
>>>>> stable vanilla 6.10, as GCC 12.3.0 complains.
>>>>>
>>>>> (Please don't throw stuff at me, I think this is the last one for today :-)
>>>>>
>>>>> arch/x86/include/asm/mshyperv.h
>>>>> -------------------------------
>>>>>   242 static inline struct hv_vp_assist_page *hv_get_vp_assist_page(unsigned int cpu)
>>>>>   243 {
>>>>>   244         if (!hv_vp_assist_page)
>>>>>   245                 return NULL;
>>>>>   246 
>>>>>   247         return hv_vp_assist_page[cpu];
>>>>>   248 }
>>>>>
>>>>> arch/x86/kvm/vmx/vmx_onhyperv.h
>>>>> -------------------------------
>>>>>   102 static inline void evmcs_load(u64 phys_addr)
>>>>>   103 {
>>>>>   104         struct hv_vp_assist_page *vp_ap =
>>>>>   105                 hv_get_vp_assist_page(smp_processor_id());
>>>>>   106 
>>>>>   107         if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
>>>>>   108                 vp_ap->nested_control.features.directhypercall = 1;
>>>>>   109         vp_ap->current_nested_vmcs = phys_addr;
>>>>>   110         vp_ap->enlighten_vmentry = 1;
>>>>>   111 }
>>>>>
>> 
>> ...
>> 
>>>
>>> GCC 12.3.0 appears unaware of this fact that evmcs_load() cannot be called with hv_vp_assist_page() == NULL.
>>>
>>> This, for example, silences the warning and also hardens the code against the "impossible" situations:
>>>
>>> -------------------><------------------------------------------------------------------
>>> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
>>> index eb48153bfd73..8b0e3ffa7fc1 100644
>>> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h
>>> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
>>> @@ -104,6 +104,11 @@ static inline void evmcs_load(u64 phys_addr)
>>>         struct hv_vp_assist_page *vp_ap =
>>>                 hv_get_vp_assist_page(smp_processor_id());
>>>  
>>> +       if (!vp_ap) {
>>> +               pr_warn("BUG: hy_get_vp_assist_page(%d) returned NULL.\n", smp_processor_id());
>>> +               return;
>>> +       }
>>> +
>>>         if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
>>>                 vp_ap->nested_control.features.directhypercall = 1;
>>>         vp_ap->current_nested_vmcs = phys_addr;
>> 
>> As Sean said, this does not seem to be possible today but I uderstand
>> why the compiler is not able to infer this. If we were to fix this, I'd
>> suggest we do something like "BUG_ON(!vp_ap)" (with a comment why)
>> instead of the suggested patch:
>
> That sounds awesome, but I really dare not poke into KVM stuff at my level. :-/
>

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);
+
        if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
                vp_ap->nested_control.features.directhypercall = 1;
        vp_ap->current_nested_vmcs = phys_addr;

the BUG_ON() will silence compiler warning as well as become a sentinel
for future code changes.

-- 
Vitaly