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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Tue, May 13, 2025 at 3:27 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 12.05.2025 16:46, Ross Lagerwall wrote:
> > 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.
>
> Yet if we start checking the value, I think the public header should also
> be made say so (in a comment).
>
> > --- a/xen/drivers/acpi/pmstat.c
> > +++ b/xen/drivers/acpi/pmstat.c
> > @@ -103,8 +103,10 @@ 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 ( copy_to_guest(op->u.getpx.trans_pt, pxpt->u.trans_pt, ct*ct) )
> > +        ct = min_t(uint32_t, pmpt->perf.state_count, op->u.getpx.total);
>
> With this, ...
>
> > +        if ( ct <= op->u.getpx.total &&
>
> ... this is going to be always true, isn't it? Which constitutes a violation
> of Misra rule 14.3.
>
> Also it would be nice if the min_t() could become a normal min(), e.g. by
> "promoting" op->u.getpx.total to unsigned int via adding 0U. This way it's
> clear there's no hidden truncation (or else there might be an argument for
> keeping the check above).
>
> > +             copy_to_guest(op->u.getpx.trans_pt, pxpt->u.trans_pt, ct * 
> > ct) )
> >          {
> >              spin_unlock(cpufreq_statistic_lock);
> >              ret = -EFAULT;
>
> Why would you constrain this copy-out but not the one just out of context
> below here? (The question is of course moot if the condition was dropped.)
>

Oh, I had intended this condition to be...

    if ( ct == op->u.getpx.total &&

... based on your previous comment about the difficulties of partially
copying a 2d array.

> An option may be to document that this array is copied only when the
> buffer is large enough.

I left the other alone since partially copying a 1d array makes sense.

If you would prefer, I can drop the condition and just let the caller
deal with the partially copied 2d array?

Thanks,
Ross