| Message ID | 20250417103000.827661-1-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/pmstat: Check size of PMSTAT_get_pxstat buffers | expand |
On 17.04.2025 12:30, Ross Lagerwall wrote: > --- a/xen/drivers/acpi/pmstat.c > +++ b/xen/drivers/acpi/pmstat.c > @@ -104,6 +104,14 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op) > cpufreq_residency_update(op->cpuid, pxpt->u.cur); > > ct = pmpt->perf.state_count; > + > + if ( op->u.getpx.total < ct ) > + { > + spin_unlock(cpufreq_statistic_lock); > + ret = -ENOSPC; > + break; > + } Simply producing an error is not an option imo. See pmstat_get_cx_stat()'s behavior. Imo the calculation of ct wants to become ct = min(pmpt->perf.state_count, op->u.getpx.total); yet then the copying of the 2-dim array of data becomes more complicated when ct < pmpt->perf.state_count. An option may be to document that this array is copied only when the buffer is large enough. Furthermore I think it would be a good idea to also amend the public header with IN/OUT annotations for the fields which are input and output (also for the Cx part, ideally). And then - doesn't xc_pm_get_pxstat() have a similar issue? Jan
diff --git a/xen/drivers/acpi/pmstat.c b/xen/drivers/acpi/pmstat.c index c51b9ca358c2..c388b1e9a17a 100644 --- a/xen/drivers/acpi/pmstat.c +++ b/xen/drivers/acpi/pmstat.c @@ -104,6 +104,14 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op) cpufreq_residency_update(op->cpuid, pxpt->u.cur); ct = pmpt->perf.state_count; + + if ( op->u.getpx.total < ct ) + { + spin_unlock(cpufreq_statistic_lock); + ret = -ENOSPC; + break; + } + if ( copy_to_guest(op->u.getpx.trans_pt, pxpt->u.trans_pt, ct*ct) ) { spin_unlock(cpufreq_statistic_lock);
Check that the total number of states passed in and hence the size of buffers is sufficient to avoid writing more than the caller has allocated. The interface is not explicit about whether getpx.total is expected to be set by the caller in this case but since it is always set in libxenctrl it seems reasonable to check it. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> Fixes: c06a7db0c547 ("X86 and IA64: Update cpufreq statistic logic for supporting both x86 and ia64") --- xen/drivers/acpi/pmstat.c | 8 ++++++++ 1 file changed, 8 insertions(+)