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
