Re: [kvm-unit-tests patch v6 05/18] x86: pmu: Enlarge cnt[] length to 48 in check_counters_many()

[Sean Christopherson <seanjc@google.com>]

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@google.com>
> >> Signed-off-by: Dapeng Mi <dapeng1.mi@linux.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".