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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALMp9eStzLK7kQY41b37zvZuR7UVzOD+W7vDPhyKXYPDhUww0g@mail.gmail.com>
Date: Fri, 19 Jul 2024 13:14:50 -0700
From: Jim Mattson <jmattson@...gle.com>
To: Mirsad Todorovac <mtodorovac69@...il.com>
Cc: Sean Christopherson <seanjc@...gle.com>, 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 Fri, Jul 19, 2024 at 12:41 PM Mirsad Todorovac
<mtodorovac69@...il.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
>