Re: [PATCH] x86/pmstat: Check size of PMSTAT_get_pxstat buffers

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Thu, Apr 17, 2025 at 2:23 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Another option would be for the caller to explicitly pass in both array sizes
and Xen can copy as much as fits since in either case there would need to be a
way for Xen to return how much it has (separately) copied for both arrays. Does
that make sense?

>
> 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).

Sure.

>
> And then - doesn't xc_pm_get_pxstat() have a similar issue?
>

Indeed, I can send a patch for that.

Ross