RE: [PATCH v5 01/18] xen/pmstat: guard perf.states[] access with XEN_PX_INIT

["Penny, Zheng" <penny.zheng@xxxxxxx>]

[Public]

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, June 11, 2025 11:20 PM
> To: Penny, Zheng <penny.zheng@xxxxxxx>
> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v5 01/18] xen/pmstat: guard perf.states[] access with
> XEN_PX_INIT
>
> On 27.05.2025 10:48, Penny Zheng wrote:
> > Accessing to perf.states[] array shall not be only guarded with
> > user-defined hypercall input, so we add XEN_PX_INIT check to gain safety.
>
> What is "guarded with user-defined hypercall input"? And what safety are we
> lacking?
>
> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -228,10 +228,13 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op
> *op)
> >      ret = copy_to_guest(op->u.get_para.affected_cpus,
> >                          data, op->u.get_para.cpu_num);
> >
> > -    for ( i = 0; i < op->u.get_para.freq_num; i++ )
> > -        data[i] = pmpt->perf.states[i].core_frequency * 1000;
> > -    ret += copy_to_guest(op->u.get_para.scaling_available_frequencies,
> > -                         data, op->u.get_para.freq_num);
> > +    if ( pmpt->perf.init & XEN_PX_INIT )
> > +    {
> > +        for ( i = 0; i < op->u.get_para.freq_num; i++ )
> > +            data[i] = pmpt->perf.states[i].core_frequency * 1000;
> > +        ret += copy_to_guest(op->u.get_para.scaling_available_frequencies,
> > +                             data, op->u.get_para.freq_num);
> > +    }
>
> Going from just the code change: You want to avoid copying out frequency 
> values
> when none have been reported? But when none have been reported, isn't pmpt-
> >perf.state_count (against which op->u.get_para.freq_num was
> validated) simply going to be 0? If not, how would callers know that no data 
> was
> handed back to them?

I may misunderstand what you've commented on v4 patch "tools/xenpm: Print CPPC 
parameters for amd-cppc driver", quoting the discussion there,
"
This looks questionable all on its own. Where is it that ->perf.states 
allocation
is being avoided? I first thought it might be patch 06 which is related, but 
that
doesn't look to be it. In any event further down from here there is

    for ( i = 0; i < op->u.get_para.freq_num; i++ )
        data[i] = pmpt->perf.states[i].core_frequency * 1000;

i.e. an access to the array solely based on hypercall input.
"
I thought we were indicating a scenario, user accidentally writes the 
"op->u.get_para.freq_num ", and it leads to accessing out-of-range array slot 
in CPPC mode. That's the reason why I added this guard

Buit as you said at the very beginning,  op->u.get_para.freq_num is validated 
against pmpt->perf.state_count, so ig the above scenario will not happen, I'll 
delete this commit.

>
> Jan