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
