Message-ID: <824a0819-a09d-40ac-820c-f7975aee1dae@gmail.com>
Date: Fri, 19 Jul 2024 21:41:32 +0200
From: Mirsad Todorovac <mtodorovac69@...il.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 x86@...nel.org, "H. Peter Anvin" <hpa@...or.com>,
 linux-kernel@...r.kernel.org, Like Xu <likexu@...cent.com>
Subject: Re: [BUG] arch/x86/kvm/vmx/pmu_intel.c:54: error: dereference of NULL ‘pmc’ [CWE-476]



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

Powered by blists - more mailing lists