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: <b20eded4-0663-49fb-ba88-5ff002a38a7f@gmail.com>
Date: Tue, 13 Aug 2024 22:33:02 +0200
From: Mirsad Todorovac <mtodorovac69@...il.com>
To: Vitaly Kuznetsov <vkuznets@...hat.com>,
 Sean Christopherson <seanjc@...gle.com>
Cc: 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 7/29/24 15:31, Vitaly Kuznetsov wrote:
> Mirsad Todorovac <mtodorovac69@...il.com> 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. :-/

> - pr_warn() is not ratelimited

This is indeed a problem. I did not see that coming.

> - 'return' from evmcs_load does not propagate the error so the VM is
> going to misbehave somewhere else.

Agreed. But, frankly, I do not see where to jump or return to in case of such bug.

I would just feel safer with a sentinel or a return value check, just as in userland some
people expect malloc() to always succeed - but the diligent check return value of this and
all syscalls. ;-)

Best regards,
Mirsad Todorovac