Re: [BUG] arch/x86/kvm/vmx/pmu_intel.c:54: error: dereference of NULL ‘pmc’ [CWE-476]

[Mirsad Todorovac <mtodorovac69@gmail.com>]

On 7/19/24 19:21, Sean Christopherson wrote:
> On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
>> Hi,
>>
>> In the build of 6.10.0 from stable tree, the following error was detected.
>>
>> You see that the function get_fixed_pmc() can return NULL pointer as a result
>> if msr is outside of [base, base + pmu->nr_arch_fixed_counters) interval.
>>
>> kvm_pmu_request_counter_reprogram(pmc) is then called with that NULL pointer
>> as the argument, which expands to .../pmu.h
>>
>> #define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)
>>
>> which is a NULL pointer dereference in that speculative case.
> 
> I'm somewhat confused.  Did you actually hit a BUG() due to a NULL-pointer
> dereference, are you speculating that there's a bug, or did you find some speculation
> issue with the CPU?
> 
> It should be impossible for get_fixed_pmc() to return NULL in this case.  The
> loop iteration is fully controlled by KVM, i.e. 'i' is guaranteed to be in the
> ranage [0..pmu->nr_arch_fixed_counters).
> 
> And the input @msr is "MSR_CORE_PERF_FIXED_CTR0 +i", so the if-statement expands to:
> 
> 	if (MSR_CORE_PERF_FIXED_CTR0 + [0..pmu->nr_arch_fixed_counters) >= MSR_CORE_PERF_FIXED_CTR0 &&
> 	    MSR_CORE_PERF_FIXED_CTR0 + [0..pmu->nr_arch_fixed_counters) < MSR_CORE_PERF_FIXED_CTR0 + pmu->nr_arch_fixed_counters)
> 
> i.e. is guaranteed to evaluate true.
> 
> Am I missing something?

Hi Sean,

Thank you for replying promptly.

Perhaps I should have provided the GCC error report in the first place. It seems that GCC 12.3.0
cannot track dependencies that deep, so it assumes that code

157         if (msr >= base && msr < base + pmu->nr_arch_fixed_counters) {
158                 u32 index = array_index_nospec(msr - base,
159                                                pmu->nr_arch_fixed_counters);
160 
161                 return &pmu->fixed_counters[index];
162         }
163 
164         return NULL;

can end up returning NULL, resulting in NULL pointer dereference.

The analyzer sees pmu->nr_arch_fixed_counters, but I am not sure that GCC can search
changes that deep.

Maybe if clause in arch/x86/kvm/vmx/../pmu.h:157 is always true, but GCC cannot see that?

Best regards,
Mirsad Todorovac

---------------------------------------------------------------------------------------
In file included from arch/x86/kvm/vmx/capabilities.h:9,
                 from arch/x86/kvm/vmx/vmcs.h:12,
                 from arch/x86/kvm/vmx/vmcs12.h:7,
                 from arch/x86/kvm/vmx/hyperv.h:6,
                 from arch/x86/kvm/vmx/nested.h:6,
                 from arch/x86/kvm/vmx/pmu_intel.c:20:
arch/x86/kvm/vmx/../pmu.h: In function ‘kvm_pmu_request_counter_reprogram’:
arch/x86/kvm/vmx/../pmu.h:11:34: error: dereference of NULL ‘pmc’ [CWE-476] [-Werror=analyzer-null-dereference]
   11 | #define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)
      |                             ~~~~~^~~~~~
arch/x86/kvm/vmx/../pmu.h:230:27: note: in expansion of macro ‘pmc_to_pmu’
  230 |         set_bit(pmc->idx, pmc_to_pmu(pmc)->reprogram_pmi);
      |                           ^~~~~~~~~~
  ‘reprogram_fixed_counters’: events 1-4
    |
    |arch/x86/kvm/vmx/pmu_intel.c:37:13:
    |   37 | static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
    |      |             ^~~~~~~~~~~~~~~~~~~~~~~~
    |      |             |
    |      |             (1) entry to ‘reprogram_fixed_counters’
    |......
    |   44 |         for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
    |      |                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                       |
    |      |                       (2) following ‘true’ branch...
    |   45 |                 u8 new_ctrl = fixed_ctrl_field(data, i);
    |      |                 ~~ ~~~~~~~~
    |      |                 |  |
    |      |                 |  (4) following ‘false’ branch...
    |      |                 (3) ...to here
    |
  ‘reprogram_fixed_counters’: event 5
    |
    |arch/x86/kvm/vmx/../pmu.h:17:54:
    |   17 | #define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
    |      |                                                      ^~
    |      |                                                      |
    |      |                                                      (5) ...to here
arch/x86/kvm/vmx/pmu_intel.c:45:31: note: in expansion of macro ‘fixed_ctrl_field’
    |   45 |                 u8 new_ctrl = fixed_ctrl_field(data, i);
    |      |                               ^~~~~~~~~~~~~~~~
    |
  ‘reprogram_fixed_counters’: event 6
    |
    |   46 |                 u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
    |      |                    ^~~~~~~~
    |      |                    |
    |      |                    (6) following ‘false’ branch...
    |
  ‘reprogram_fixed_counters’: event 7
    |
    |arch/x86/kvm/vmx/../pmu.h:17:54:
    |   17 | #define fixed_ctrl_field(ctrl_reg, idx) (((ctrl_reg) >> ((idx)*4)) & 0xf)
    |      |                                                      ^~
    |      |                                                      |
    |      |                                                      (7) ...to here
arch/x86/kvm/vmx/pmu_intel.c:46:31: note: in expansion of macro ‘fixed_ctrl_field’
    |   46 |                 u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
    |      |                               ^~~~~~~~~~~~~~~~
    |
  ‘reprogram_fixed_counters’: events 8-9
    |
    |   48 |                 if (old_ctrl == new_ctrl)
    |      |                    ^
    |      |                    |
    |      |                    (8) following ‘false’ branch...
    |......
    |   51 |                 pmc = get_fixed_pmc(pmu, MSR_CORE_PERF_FIXED_CTR0 + i);
    |      |                 ~~~ 
    |      |                 |
    |      |                 (9) ...to here
    |
  ‘reprogram_fixed_counters’: events 10-12
    |
    |arch/x86/kvm/vmx/../pmu.h:157:12:
    |  157 |         if (msr >= base && msr < base + pmu->nr_arch_fixed_counters) {
    |      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |            |            |                  |
    |      |            |            |                  (11) ...to here
    |      |            |            (12) following ‘false’ branch...
    |      |            (10) following ‘true’ branch...
    |
  ‘reprogram_fixed_counters’: events 13-14
    |
    |arch/x86/kvm/vmx/pmu_intel.c:51:23:
    |   51 |                 pmc = get_fixed_pmc(pmu, MSR_CORE_PERF_FIXED_CTR0 + i);
    |      |                       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                       |
    |      |                       (13) ...to here
    |......
    |   54 |                 kvm_pmu_request_counter_reprogram(pmc);
    |      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    |      |                 |
    |      |                 (14) calling ‘kvm_pmu_request_counter_reprogram’ from ‘reprogram_fixed_counters’
    |
    +--> ‘kvm_pmu_request_counter_reprogram’: event 15
           |
           |arch/x86/kvm/vmx/../pmu.h:228:20:
           |  228 | static inline void kvm_pmu_request_counter_reprogram(struct kvm_pmc *pmc)
           |      |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           |      |                    |
           |      |                    (15) entry to ‘kvm_pmu_request_counter_reprogram’
           |
         ‘kvm_pmu_request_counter_reprogram’: event 16
           |
           |   11 | #define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)
           |      |                             ~~~~~^~~~~~
           |      |                                  |
           |      |                                  (16) dereference of NULL ‘pmc’
arch/x86/kvm/vmx/../pmu.h:230:27: note: in expansion of macro ‘pmc_to_pmu’
           |  230 |         set_bit(pmc->idx, pmc_to_pmu(pmc)->reprogram_pmi);
           |      |                           ^~~~~~~~~~
           |
cc1: all warnings being treated as errors


>> arch/x86/kvm/vmx/pmu_intel.c
>> ----------------------------
>>  37 static void reprogram_fixed_counters(struct kvm_pmu *pmu, u64 data)
>>  38 {
>>  39         struct kvm_pmc *pmc;
>>  40         u64 old_fixed_ctr_ctrl = pmu->fixed_ctr_ctrl;
>>  41         int i;
>>  42 
>>  43         pmu->fixed_ctr_ctrl = data;
>>  44         for (i = 0; i < pmu->nr_arch_fixed_counters; i++) {
>>  45                 u8 new_ctrl = fixed_ctrl_field(data, i);
>>  46                 u8 old_ctrl = fixed_ctrl_field(old_fixed_ctr_ctrl, i);
>>  47 
>>  48                 if (old_ctrl == new_ctrl)
>>  49                         continue;
>>  50 
>>  51 →               pmc = get_fixed_pmc(pmu, MSR_CORE_PERF_FIXED_CTR0 + i);
>>  52 
>>  53                 __set_bit(KVM_FIXED_PMC_BASE_IDX + i, pmu->pmc_in_use);
>>  54 →               kvm_pmu_request_counter_reprogram(pmc);
>>  55         }
>>  56 }
>> ----------------------------
>>
>> arch/x86/kvm/vmx/../pmu.h
>> -------------------------
>>  11 #define pmc_to_pmu(pmc)   (&(pmc)->vcpu->arch.pmu)
>> .
>> .
>> .
>> 152 /* returns fixed PMC with the specified MSR */
>> 153 static inline struct kvm_pmc *get_fixed_pmc(struct kvm_pmu *pmu, u32 msr)
>> 154 {
>> 155         int base = MSR_CORE_PERF_FIXED_CTR0;
>> 156 
>> 157         if (msr >= base && msr < base + pmu->nr_arch_fixed_counters) {
>> 158                 u32 index = array_index_nospec(msr - base,
>> 159                                                pmu->nr_arch_fixed_counters);
>> 160 
>> 161                 return &pmu->fixed_counters[index];
>> 162         }
>> 163 
>> 164         return NULL;
>> 165 }
>> .
>> .
>> .
>> 228 static inline void kvm_pmu_request_counter_reprogram(struct kvm_pmc *pmc)
>> 229 {
>> 230         set_bit(pmc->idx, pmc_to_pmu(pmc)->reprogram_pmi);
>> 231         kvm_make_request(KVM_REQ_PMU, pmc->vcpu);
>> 232 }
>> .
>> .
>> .
>> -------------------------
>> 76d287b2342e1
>> Offending commits are: 76d287b2342e1 and 4fa5843d81fdc.
>>
>> I am not familiar with this subset of code, so I do not know the right code to implement
>> for the case get_fixed_pmc(pmu, MSR_CORE_PERF_FIXED_CTR0 + i) returns NULL.
>>
>> Hope this helps.
>>
>> Best regards,
>> Mirsad Todorovac