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
