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.
Signed-off-by: Penny Zheng <Penny.Zheng@amd.com>
---
v4 -> v5:
- new commit
---
xen/drivers/acpi/pmstat.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c
index c51b9ca358..b7fcc02db9 100644
--- 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);
+ }
xfree(data);
if ( ret )
--
2.34.1On 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?
Jan
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, June 11, 2025 11:20 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@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
On 16.06.2025 11:05, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Wednesday, June 11, 2025 11:20 PM
>> To: Penny, Zheng <penny.zheng@amd.com>
>> Cc: Huang, Ray <Ray.Huang@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
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?"
> 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
[Public]
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Monday, June 16, 2025 5:16 PM
> To: Penny, Zheng <penny.zheng@amd.com>
> Cc: Huang, Ray <Ray.Huang@amd.com>; xen-devel@lists.xenproject.org
> 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@suse.com>
> >> Sent: Wednesday, June 11, 2025 11:20 PM
> >> To: Penny, Zheng <penny.zheng@amd.com>
> >> Cc: Huang, Ray <Ray.Huang@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
>
> 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
© 2016 - 2025 Red Hat, Inc.