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: Monday, June 16, 2025 5:16 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 16.06.2025 11:05, Penny, Zheng wrote:
> > [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
>
> Well, it's not an out-of-range access, but a NULL deref, but yes, the concern 
> voiced
> there is related. But that can't be done in an isolated patch, it makes sense 
> only
> together with the change to the if() that I did comment on. And even then if 
> it is
> guaranteed that perf.state_count is always 0 when perf.states is NULL, we'd 
> be fine
> without any change. Yet this latter aspect goes back to the question I had 
> raised
> there: "Where is it that ->perf.states allocation is being avoided?"
>

->perf.states is allocated in set_px_pminfo(), which is a px-specific function.
In the caller of set_px_pminfo(), we have assertion 
"ASSERT(!(xen_processor_pmbits & XEN_PROCESSOR_PM_CPPC));"
to ensure it will not be invoked in CPPC mode.

Answering" And even then if it is  guaranteed that perf.state_count is always 0 
when perf.states is NULL", maybe we shall add another check
in the very beginning of get_cpufreq_para():
```
if ( !pmpt || ((pmpt->init & XEN_PX_INIT) && !pmpt->perf.states) || 
((pmpt->init & XEN_CPPC_INIT) && pmpt->perf.state_count))
```
We are ensuring in CPPC mode, ->perf.state_count must be zero, then 
op->u.get_para.freq_num is validated against it, we will
never enter into the loop...

And if we're okay with above change, right, it shall not be done in an isolated 
patch, and it shall be added only together with the change to the if()

> > 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.
>
> Not sure yet whether deleting is the right course of action.
>
> Jan