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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan