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

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

[Mirsad Todorovac]

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.

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

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

[Sean Christopherson]

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

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

[Mirsad Todorovac]

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

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

[Sean Christopherson]

On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
> 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.

Yes, though the report itself is somewhat secondary, what matters the most is how
you found the bug and how to reproduce the failure.  Critically, IIUC, this requires
analyzer-null-dereference, which AFAIK isn't even enabled by W=1, let alone a base
build.

Please see the 0-day bot's reports[*] for a fantastic example of how to report
things that are found by non-standard (by kernel standards) means.

In general, I suspect that analyzer-null-dereference will generate a _lot_ of
false positives, and is probably not worth reporting unless you are absolutely
100% certain there's a real bug.  I (and most maintainers) am happy to deal with
false positives here and there _if_ the signal to noise ratio is high.  But if
most reports are false positives, they'll likely all end up getting ignored.

[*] https://lore.kernel.org/all/202406111250.d8XtA9SC-lkp@intel.com

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

[Mirsad Todorovac]

On 7/19/24 21:22, Sean Christopherson wrote:
> On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
>> 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.
> 
> Yes, though the report itself is somewhat secondary, what matters the most is how
> you found the bug and how to reproduce the failure.  Critically, IIUC, this requires
> analyzer-null-dereference, which AFAIK isn't even enabled by W=1, let alone a base
> build.
> 
> Please see the 0-day bot's reports[*] for a fantastic example of how to report
> things that are found by non-standard (by kernel standards) means.
> 
> In general, I suspect that analyzer-null-dereference will generate a _lot_ of
> false positives, and is probably not worth reporting unless you are absolutely
> 100% certain there's a real bug.  I (and most maintainers) am happy to deal with
> false positives here and there _if_ the signal to noise ratio is high.  But if
> most reports are false positives, they'll likely all end up getting ignored.
> 
> [*] https://lore.kernel.org/all/202406111250.d8XtA9SC-lkp@intel.com

I think I understood the meaning between the lines.

However, to repeat the obvious, reducing the global dependencies simplifies the readability
and the logical proof of the code. :-/

Needless to say, dividing into pure functions and const functions reduces the number of
dependencies, as it is N × (N - 1), sqr (N).

For example, if a condition is always true, but the compiler cannot deduce it from code,
there is something odd.

CONCLUSION: If this generated 5 out of 5 false positives, then I might be giving up on this
as a waste of your time.

However, it was great fun analysing x86 KVM code. :-)

Sort of cool that you guys on Google consider bug report from nobody admins from the
universities ;-)

Best regards,
Mirsad Todorovac

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

[Jim Mattson]

On Fri, Jul 19, 2024 at 12:41 PM Mirsad Todorovac
<mtodorovac69@gmail.com> wrote:
>
>
>
> On 7/19/24 21:22, Sean Christopherson wrote:
> > On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
> >> 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.
> >
> > Yes, though the report itself is somewhat secondary, what matters the most is how
> > you found the bug and how to reproduce the failure.  Critically, IIUC, this requires
> > analyzer-null-dereference, which AFAIK isn't even enabled by W=1, let alone a base
> > build.
> >
> > Please see the 0-day bot's reports[*] for a fantastic example of how to report
> > things that are found by non-standard (by kernel standards) means.
> >
> > In general, I suspect that analyzer-null-dereference will generate a _lot_ of
> > false positives, and is probably not worth reporting unless you are absolutely
> > 100% certain there's a real bug.  I (and most maintainers) am happy to deal with
> > false positives here and there _if_ the signal to noise ratio is high.  But if
> > most reports are false positives, they'll likely all end up getting ignored.
> >
> > [*] https://lore.kernel.org/all/202406111250.d8XtA9SC-lkp@intel.com
>
> I think I understood the meaning between the lines.
>
> However, to repeat the obvious, reducing the global dependencies simplifies the readability
> and the logical proof of the code. :-/

Comments would also help. :)

> Needless to say, dividing into pure functions and const functions reduces the number of
> dependencies, as it is N × (N - 1), sqr (N).
>
> For example, if a condition is always true, but the compiler cannot deduce it from code,
> there is something odd.
>
> CONCLUSION: If this generated 5 out of 5 false positives, then I might be giving up on this
> as a waste of your time.
>
> However, it was great fun analysing x86 KVM code. :-)

I assure you that there are plenty of actual bugs in KVM. This tool
just isn't finding them.

> Sort of cool that you guys on Google consider bug report from nobody admins from the
> universities ;-)
>
> Best regards,
> Mirsad Todorovac
>

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

[Mirsad Todorovac]

On 7/19/24 22:14, Jim Mattson wrote:
> On Fri, Jul 19, 2024 at 12:41 PM Mirsad Todorovac
> <mtodorovac69@gmail.com> wrote:
>>
>>
>>
>> On 7/19/24 21:22, Sean Christopherson wrote:
>>> On Fri, Jul 19, 2024, Mirsad Todorovac wrote:
>>>> 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.
>>>
>>> Yes, though the report itself is somewhat secondary, what matters the most is how
>>> you found the bug and how to reproduce the failure.  Critically, IIUC, this requires
>>> analyzer-null-dereference, which AFAIK isn't even enabled by W=1, let alone a base
>>> build.
>>>
>>> Please see the 0-day bot's reports[*] for a fantastic example of how to report
>>> things that are found by non-standard (by kernel standards) means.
>>>
>>> In general, I suspect that analyzer-null-dereference will generate a _lot_ of
>>> false positives, and is probably not worth reporting unless you are absolutely
>>> 100% certain there's a real bug.  I (and most maintainers) am happy to deal with
>>> false positives here and there _if_ the signal to noise ratio is high.  But if
>>> most reports are false positives, they'll likely all end up getting ignored.
>>>
>>> [*] https://lore.kernel.org/all/202406111250.d8XtA9SC-lkp@intel.com
>>
>> I think I understood the meaning between the lines.
>>
>> However, to repeat the obvious, reducing the global dependencies simplifies the readability
>> and the logical proof of the code. :-/
> 
> Comments would also help. :)
> 
>> Needless to say, dividing into pure functions and const functions reduces the number of
>> dependencies, as it is N × (N - 1), sqr (N).
>>
>> For example, if a condition is always true, but the compiler cannot deduce it from code,
>> there is something odd.
>>
>> CONCLUSION: If this generated 5 out of 5 false positives, then I might be giving up on this
>> as a waste of your time.
>>
>> However, it was great fun analysing x86 KVM code. :-)
> 
> I assure you that there are plenty of actual bugs in KVM. This tool
> just isn't finding them.

Well, this series of reports did not target KVM. It was accidental that GCC static analyser
reported those dubious false positives first.

Best regards,
Mirsad Todorovac
 
>> Sort of cool that you guys on Google consider bug report from nobody admins from the
>> universities ;-)
>>
>> Best regards,
>> Mirsad Todorovac
>>