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

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

[Mirsad Todorovac]

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: 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().

Commits 50a82b0eb88c1 and a46d15cc1ae5a are related to the issue.

Hope this helps.

Best regards,
Mirsad Todorovac

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

[Sean Christopherson]

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().

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

[Mirsad Todorovac]

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

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

[Vitaly Kuznetsov]

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

-- 
Vitaly

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

[Mirsad Todorovac]

On 7/29/24 15:31, Vitaly Kuznetsov wrote:
> Mirsad Todorovac <mtodorovac69@gmail.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

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

[Vitaly Kuznetsov]

Mirsad Todorovac <mtodorovac69@gmail.com> writes:

> On 7/29/24 15:31, Vitaly Kuznetsov wrote:
>> Mirsad Todorovac <mtodorovac69@gmail.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. :-/
>

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

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

[Sean Christopherson]

On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> 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);

A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
check here and do more than just WARN, then it's easy enough to plumb in @vcpu
and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
corrupting the guest somehow.

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

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
>> 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);
>
> A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
> check here and do more than just WARN, then it's easy enough to plumb in @vcpu
> and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
> corrupting the guest somehow.
>

I'm still acting under the impression this is an absolutely impossible
situation :-)

AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't
have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the
plumbing (or am I missing an easy way to go back from @vmcs to
@vcpu?). On the other hand, vmcs_load() should not be called that ofter
so if we prefer to have @vcpu there for some other reason -- why not.

-- 
Vitaly

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

[Sean Christopherson]

On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> >> 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);
> >
> > A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
> > check here and do more than just WARN, then it's easy enough to plumb in @vcpu
> > and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
> > corrupting the guest somehow.
> >
> 
> I'm still acting under the impression this is an absolutely impossible
> situation :-)
> 
> AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't
> have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the
> plumbing (or am I missing an easy way to go back from @vmcs to
> @vcpu?). On the other hand, vmcs_load() should not be called that ofter
> so if we prefer to have @vcpu there for some other reason -- why not.

kvm_get_running_vcpu(), though I honestly purposely didn't suggest it earlier
because I am not a fan of using kvm_get_running_vcpu() unless it's absolutely
necessary.  But for this situation, I'd be fine with using it.

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

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
>> Sean Christopherson <seanjc@google.com> writes:
>> 
>> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
>> >> 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);
>> >
>> > A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
>> > check here and do more than just WARN, then it's easy enough to plumb in @vcpu
>> > and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
>> > corrupting the guest somehow.
>> >
>> 
>> I'm still acting under the impression this is an absolutely impossible
>> situation :-)
>> 
>> AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't
>> have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the
>> plumbing (or am I missing an easy way to go back from @vmcs to
>> @vcpu?). On the other hand, vmcs_load() should not be called that ofter
>> so if we prefer to have @vcpu there for some other reason -- why not.
>
> kvm_get_running_vcpu(), though I honestly purposely didn't suggest it earlier
> because I am not a fan of using kvm_get_running_vcpu() unless it's absolutely
> necessary.  But for this situation, I'd be fine with using it.

Ah, nice, so we don't even need the plumbing then I guess? Compile-tested only:

diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
index eb48153bfd73..318f5f95f211 100644
--- a/arch/x86/kvm/vmx/vmx_onhyperv.h
+++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
@@ -104,6 +104,19 @@ 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.
+        */
+       if (!vp_ap) {
+               struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
+
+               KVM_BUG_ON(1, vcpu->kvm);
+               return;
+       }
+
        if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
                vp_ap->nested_control.features.directhypercall = 1;
        vp_ap->current_nested_vmcs = phys_addr;

(I hope we can't reach here with kvm_running_vcpu unset, can we?)

-- 
Vitaly

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

[Sean Christopherson]

On Thu, Aug 15, 2024, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> >> Sean Christopherson <seanjc@google.com> writes:
> >> 
> >> > On Wed, Aug 14, 2024, Vitaly Kuznetsov wrote:
> >> >> 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);
> >> >
> >> > A full BUG_ON() is overkill, and easily avoided.  If we want to add a sanity
> >> > check here and do more than just WARN, then it's easy enough to plumb in @vcpu
> >> > and make this a KVM_BUG_ON() so that the VM dies, i.e. so that KVM doesn't risk
> >> > corrupting the guest somehow.
> >> >
> >> 
> >> I'm still acting under the impression this is an absolutely impossible
> >> situation :-)
> >> 
> >> AFAICS, we only call evmcs_load() from vmcs_load() but this one doesn't
> >> have @vcpu/@kvm either and I wasn't sure it's worth the effort to do the
> >> plumbing (or am I missing an easy way to go back from @vmcs to
> >> @vcpu?). On the other hand, vmcs_load() should not be called that ofter
> >> so if we prefer to have @vcpu there for some other reason -- why not.
> >
> > kvm_get_running_vcpu(), though I honestly purposely didn't suggest it earlier
> > because I am not a fan of using kvm_get_running_vcpu() unless it's absolutely
> > necessary.  But for this situation, I'd be fine with using it.
> 
> Ah, nice, so we don't even need the plumbing then I guess? Compile-tested only:
> 
> diff --git a/arch/x86/kvm/vmx/vmx_onhyperv.h b/arch/x86/kvm/vmx/vmx_onhyperv.h
> index eb48153bfd73..318f5f95f211 100644
> --- a/arch/x86/kvm/vmx/vmx_onhyperv.h
> +++ b/arch/x86/kvm/vmx/vmx_onhyperv.h
> @@ -104,6 +104,19 @@ 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.
> +        */
> +       if (!vp_ap) {
> +               struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> +
> +               KVM_BUG_ON(1, vcpu->kvm);
> +               return;

Eh, I would just do:

	if (KVM_BUG_ON(!vp_ap, kvm_get_running_vcpu()->kvm))
		return

> +       }
> +
>         if (current_evmcs->hv_enlightenments_control.nested_flush_hypercall)
>                 vp_ap->nested_control.features.directhypercall = 1;
>         vp_ap->current_nested_vmcs = phys_addr;
> 
> (I hope we can't reach here with kvm_running_vcpu unset, can we?)

Yes?  kvm_running_vcpu is set before kvm_arch_vcpu_load() and cleared after
kvm_arch_vcpu_put(), and I can't think of a scenario where it would be legal/sane
to invoke vmcs_load() without a running/loaded vCPU.  VMX needs the current VMCS
to be loaded to ensure guest state can be accessed, so any ioctl() that can touch
guest state needs to do vcpu_load().

x86's kvm_arch_vcpu_ioctl() unconditionally does vcpu_load(), and the only ioctls
I see in kvm_vcpu_ioctl() that _don't_ do vcpu_load() are KVM_SET_SIGNAL_MASK and
KVM_GET_STATS_FD, so I think we're good.

And if I'm wrong and the impossible happens twice, so be it, we die on #GP :-)