linux-kernel - Re: [kvm-unit-tests patch v6 05/18] x86: pmu: Enlarge cnt[] length to 48 in check_counters

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] [day] [month] [year] [list]

Message-ID: <Z7Stmz1VUE-cZUzq@google.com>
Date: Tue, 18 Feb 2025 07:56:11 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Dapeng Mi <dapeng1.mi@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Jim Mattson <jmattson@...gle.com>, Mingwei Zhang <mizhang@...gle.com>, 
	Xiong Zhang <xiong.y.zhang@...el.com>, Zhenyu Wang <zhenyuw@...ux.intel.com>, 
	Like Xu <like.xu.linux@...il.com>, Jinrong Liang <cloudliang@...cent.com>, 
	Yongwei Ma <yongwei.ma@...el.com>, Dapeng Mi <dapeng1.mi@...el.com>
Subject: Re: [kvm-unit-tests patch v6 05/18] x86: pmu: Enlarge cnt[] length to
 48 in check_counters_many()

On Tue, Feb 18, 2025, Dapeng Mi wrote:
> On 2/15/2025 5:06 AM, Sean Christopherson wrote:
> > On Sat, Sep 14, 2024, Dapeng Mi wrote:
> >> Considering there are already 8 GP counters and 4 fixed counters on
> >> latest Intel processors, like Sapphire Rapids. The original cnt[] array
> >> length 10 is definitely not enough to cover all supported PMU counters on
> >> these new processors even through currently KVM only supports 3 fixed
> >> counters at most. This would cause out of bound memory access and may trigger
> >> false alarm on PMU counter validation
> >>
> >> It's probably more and more GP and fixed counters are introduced in the
> >> future and then directly extends the cnt[] array length to 48 once and
> >> for all. Base on the layout of IA32_PERF_GLOBAL_CTRL and
> >> IA32_PERF_GLOBAL_STATUS, 48 looks enough in near feature.
> >>
> >> Reviewed-by: Jim Mattson <jmattson@...gle.com>
> >> Signed-off-by: Dapeng Mi <dapeng1.mi@...ux.intel.com>
> >> ---
> >>  x86/pmu.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/x86/pmu.c b/x86/pmu.c
> >> index a0268db8..b4de2680 100644
> >> --- a/x86/pmu.c
> >> +++ b/x86/pmu.c
> >> @@ -255,7 +255,7 @@ static void check_fixed_counters(void)
> >>  
> >>  static void check_counters_many(void)
> >>  {
> >> -	pmu_counter_t cnt[10];
> >> +	pmu_counter_t cnt[48];
> > ARGH.  Since the *entire* purpose of increasing the size is to guard against
> > buffer overflow, add an assert that the loop doesn't overflow.
> 
> This is not only for ensuring no buffer overflow.

In practice, it is.  As is, there are *zero* sanity checks or restrictions on the
number of possible counters.  Yes, the net effect is that the test doesn't work
if a CPU supports more than ARRAY_SIZE(cnt) counters, but the reason the test
doesn't work is because such a CPU would cause buffer overflow.

Yes, there are more nuanced reasons for using a large, statically sized array.
If the goal was to support any theoretical CPU, the array would be dynamically
allocated, but that's not worth the complexity.  If the goal just was to support
SPR, the size would have been set to 12, but that would incur additional maintenance
in the not-too-distant future.

> As the commit message says,  the counter number has already exceeded 10, such
> as SPR has 12 counters (8 GP + 4 fixed),

I am well aware.

> and there would be more counters in later platfroms. The aim of enlarging the
> array size is to ensure these counters can be enabled and verified
> simultaneously.  48 may be too large and 32 should be fair enough. Thanks.

No.  Just no.  Unless there is an architecturally defined limit, and even then a
sanity check is strongly encourage, KVM-related software should never, ever blindly
assume a buffer size is "good enough".