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
