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

[Sean Christopherson <seanjc@google.com>]

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?

> 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