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

[Sean Christopherson <seanjc@google.com>]

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