[Public]
> -----Original Message-----
> From: Jan Beulich <jbeul...@suse.com>
> Sent: Wednesday, June 11, 2025 11:20 PM
> To: Penny, Zheng <penny.zh...@amd.com>
> Cc: Huang, Ray <ray.hu...@amd.com>; xen-devel@lists.xenproject.org
> 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
