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

[Mirsad Todorovac <mtodorovac69@gmail.com>]

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 }
>>
>> Now, this one is simple:
> 
> Nope :-)
> 
>> hv_vp_assist_page(cpu) can return NULL, and in line 104 it is assigned to
>> wp_ap, which is dereferenced in lines 108, 109, and 110, which is not checked
>> against returning NULL by hv_vp_assist_page().
> 
> When enabling eVMCS, and when onlining a CPU with eVMCS enabled, KVM verifies
> that every CPU has a valid hv_vp_assist_page() and either aborts enabling eVMCS
> or rejects CPU onlining.  So very subtly, it's impossible for hv_vp_assist_page()
> to be NULL at evmcs_load().

I see, however I did not invent it, it broke my build with CONFIG_FORTIFY_SOURCE=y.

I think I warned that I have little knowledge about the KVM code.

Here is the full GCC 12.3.0 report:

------------------------------------------------------------------------------------------------------------------
In file included from arch/x86/kvm/vmx/vmx_ops.h:9,
                 from arch/x86/kvm/vmx/vmx.h:15,
                 from arch/x86/kvm/vmx/hyperv.h:7,
                 from arch/x86/kvm/vmx/nested.c:11:
arch/x86/kvm/vmx/vmx_onhyperv.h: In function ‘evmcs_load’:
arch/x86/kvm/vmx/vmx_onhyperv.h:109:36: error: dereference of NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference]
  109 |         vp_ap->current_nested_vmcs = phys_addr;
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~
  ‘nested_release_vmcs12.part.0’: events 1-4
    |
    |arch/x86/kvm/vmx/nested.c:5306:20:
    | 5306 | static inline void nested_release_vmcs12(struct kvm_vcpu *vcpu)
    |      |                    ^~~~~~~~~~~~~~~~~~~~~
    |      |                    |
    |      |                    (1) entry to ‘nested_release_vmcs12.part.0’
    |......
    | 5315 |         if (enable_shadow_vmcs) {
    |      |            ~        
    |      |            |
    |      |            (2) following ‘true’ branch...
    |......
    | 5318 |                 copy_shadow_to_vmcs12(vmx);
    |      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                 |
    |      |                 (3) ...to here
    |      |                 (4) calling ‘copy_shadow_to_vmcs12’ from ‘nested_release_vmcs12.part.0’
    |
    +--> ‘copy_shadow_to_vmcs12’: events 5-6
           |
           | 1565 | static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
           |      |             ^~~~~~~~~~~~~~~~~~~~~
           |      |             |
           |      |             (5) entry to ‘copy_shadow_to_vmcs12’
           |......
           | 1573 |         if (WARN_ON(!shadow_vmcs))
           |      |            ~ 
           |      |            |
           |      |            (6) following ‘false’ branch...
           |
         ‘copy_shadow_to_vmcs12’: event 7
           |
           |./include/linux/preempt.h:214:1:
           |  214 | do { \
           |      | ^~
           |      | |
           |      | (7) ...to here
arch/x86/kvm/vmx/nested.c:1576:9: note: in expansion of macro ‘preempt_disable’
           | 1576 |         preempt_disable();
           |      |         ^~~~~~~~~~~~~~~
           |
         ‘copy_shadow_to_vmcs12’: event 8
           |
           | 1578 |         vmcs_load(shadow_vmcs);
           |      |         ^~~~~~~~~~~~~~~~~~~~~~
           |      |         |
           |      |         (8) calling ‘vmcs_load’ from ‘copy_shadow_to_vmcs12’
           |
           +--> ‘vmcs_load’: events 9-12
                  |
                  |arch/x86/kvm/vmx/vmx_ops.h:294:20:
                  |  294 | static inline void vmcs_load(struct vmcs *vmcs)
                  |      |                    ^~~~~~~~~
                  |      |                    |
                  |      |                    (9) entry to ‘vmcs_load’
                  |......
                  |  298 |         if (kvm_is_using_evmcs())
                  |      |            ~        
                  |      |            |
                  |      |            (10) following ‘true’ branch...
                  |  299 |                 return evmcs_load(phys_addr);
                  |      |                 ~~~~~~ ~~~~~~~~~~~~~~~~~~~~~
                  |      |                 |      |
                  |      |                 |      (12) calling ‘evmcs_load’ from ‘vmcs_load’
                  |      |                 (11) ...to here
                  |
                  +--> ‘evmcs_load’: event 13
                         |
                         |arch/x86/kvm/vmx/vmx_onhyperv.h:102:20:
                         |  102 | static inline void evmcs_load(u64 phys_addr)
                         |      |                    ^~~~~~~~~~
                         |      |                    |
                         |      |                    (13) entry to ‘evmcs_load’
                         |
                       ‘evmcs_load’: event 14
                         |
                         |./arch/x86/include/asm/mshyperv.h:244:12:
                         |  244 |         if (!hv_vp_assist_page)
                         |      |            ^
                         |      |            |
                         |      |            (14) following ‘true’ branch...
                         |
                       ‘evmcs_load’: events 15-18
                         |
                         |arch/x86/kvm/vmx/vmx_onhyperv.h:105:17:
                         |  105 |                 hv_get_vp_assist_page(smp_processor_id());
                         |      |                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                         |      |                 |
                         |      |                 (15) ...to here
                         |  106 | 
                         |  107 |         if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
                         |      |            ~     
                         |      |            |
                         |      |            (16) following ‘false’ branch...
                         |  108 |                 vp_ap->nested_control.features.directhypercall = 1;
                         |  109 |         vp_ap->current_nested_vmcs = phys_addr;
                         |      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                         |      |         |                          |
                         |      |         (17) ...to here            (18) dereference of NULL ‘<unknown>’
                         |
cc1: all warnings being treated as errors
-----------------------------------------------------------------------------------

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;
--

Best regards,
Mirsad Todorovac