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 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
>>