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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.05.2025 12:53, Ross Lagerwall wrote:
> 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.

Right, if the relation there becomes == it indeed reflects that the 2-D
one is different in this regard from the 1-D one.

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

With the adjusted relation I think all is going to be fine as you
(otherwise) had it. There may want to be a brief comment on that extra
condition you add.

Jan