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
