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/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:
- pr_warn() is not ratelimited
- 'return' from evmcs_load does not propagate the error so the VM is
going to misbehave somewhere else.

-- 
Vitaly