Re: [PATCH v2 3/5] cpufreq: Avoid potential buffer overrun and leak

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.05.2025 16:46, Ross Lagerwall wrote:
> If set_px_pminfo is called a second time with a larger state_count than
> the first call, calls to PMSTAT_get_pxstat will read beyond the end of
> the pt and trans_pt buffers allocated in cpufreq_statistic_init() since
> they would have been allocated with the original state_count.
> 
> Secondly, the states array leaks on each subsequent call of
> set_px_pminfo.
> 
> As far as I know, there is no valid reason to call set_px_pminfo
> multiple times for the same CPU so fix both these issues by disallowing
> it.

Iirc it's the processor driver in Linux which would invoke this upon being
loaded. It can be unloaded and loaded again. Will it ignore the errors in
such a case? As suggested to Penny for some of her work in this area, it
may be better to return success instead, to avoid the need for following
bad practice in drivers by ignoring errors.

> --- a/xen/drivers/cpufreq/cpufreq.c
> +++ b/xen/drivers/cpufreq/cpufreq.c
> @@ -520,7 +520,7 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *perf)
>      if ( perf->flags & XEN_PX_PSS )
>      {
>          /* capability check */
> -        if ( perf->state_count <= 1 )
> +        if ( perf->state_count <= 1 || pxpt->states )

Even without the remark above, there probably would want to be two separate
if()s, each with a distinctive comment. The comment that's there would go
partly stale by the change you suggest. Or perhaps the extra condition could
move (inverted) into the outer if()'s clause.

>          {
>              ret = -EINVAL;
>              goto out;
> @@ -534,6 +534,8 @@ int set_px_pminfo(uint32_t acpi_id, struct 
> xen_processor_performance *perf)
>          }
>          if ( copy_from_guest(pxpt->states, perf->states, perf->state_count) )
>          {
> +            xfree(pxpt->states);
> +            pxpt->states = NULL;

Please avoid open-coding XFREE().

Further related to the earlier comment: Beyond the processing of PSS there's
more processing below here. If the PSS part succeeded and some later part
failed, it may actually be necessary to invoke this operation again. I.e.
even more so relevant that it won't fail just because PSS was already
processed.

Jan