Re: [PATCH v5 01/18] xen/pmstat: guard perf.states[] access with XEN_PX_INIT

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.06.2025 11:05, Penny, Zheng wrote:
> [Public]
> 
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Wednesday, June 11, 2025 11:20 PM
>> To: Penny, Zheng <penny.zheng@xxxxxxx>
>> Cc: Huang, Ray <Ray.Huang@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v5 01/18] xen/pmstat: guard perf.states[] access with
>> XEN_PX_INIT
>>
>> On 27.05.2025 10:48, Penny Zheng wrote:
>>> Accessing to perf.states[] array shall not be only guarded with
>>> user-defined hypercall input, so we add XEN_PX_INIT check to gain safety.
>>
>> What is "guarded with user-defined hypercall input"? And what safety are we
>> lacking?
>>
>>> --- a/xen/drivers/acpi/pmstat.c
>>> +++ b/xen/drivers/acpi/pmstat.c
>>> @@ -228,10 +228,13 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op
>> *op)
>>>      ret = copy_to_guest(op->u.get_para.affected_cpus,
>>>                          data, op->u.get_para.cpu_num);
>>>
>>> -    for ( i = 0; i < op->u.get_para.freq_num; i++ )
>>> -        data[i] = pmpt->perf.states[i].core_frequency * 1000;
>>> -    ret += copy_to_guest(op->u.get_para.scaling_available_frequencies,
>>> -                         data, op->u.get_para.freq_num);
>>> +    if ( pmpt->perf.init & XEN_PX_INIT )
>>> +    {
>>> +        for ( i = 0; i < op->u.get_para.freq_num; i++ )
>>> +            data[i] = pmpt->perf.states[i].core_frequency * 1000;
>>> +        ret += copy_to_guest(op->u.get_para.scaling_available_frequencies,
>>> +                             data, op->u.get_para.freq_num);
>>> +    }
>>
>> Going from just the code change: You want to avoid copying out frequency 
>> values
>> when none have been reported? But when none have been reported, isn't pmpt-
>>> perf.state_count (against which op->u.get_para.freq_num was
>> validated) simply going to be 0? If not, how would callers know that no data 
>> was
>> handed back to them?
> 
> I may misunderstand what you've commented on v4 patch "tools/xenpm: Print 
> CPPC parameters for amd-cppc driver", quoting the discussion there,
> "
> This looks questionable all on its own. Where is it that ->perf.states 
> allocation
> is being avoided? I first thought it might be patch 06 which is related, but 
> that
> doesn't look to be it. In any event further down from here there is
> 
>     for ( i = 0; i < op->u.get_para.freq_num; i++ )
>         data[i] = pmpt->perf.states[i].core_frequency * 1000;
> 
> i.e. an access to the array solely based on hypercall input.
> "
> I thought we were indicating a scenario, user accidentally writes the 
> "op->u.get_para.freq_num ", and it leads to accessing out-of-range array slot 
> in CPPC mode. That's the reason why I added this guard

Well, it's not an out-of-range access, but a NULL deref, but yes, the concern 
voiced
there is related. But that can't be done in an isolated patch, it makes sense 
only
together with the change to the if() that I did comment on. And even then if it 
is
guaranteed that perf.state_count is always 0 when perf.states is NULL, we'd be 
fine
without any change. Yet this latter aspect goes back to the question I had 
raised
there: "Where is it that ->perf.states allocation is being avoided?"

> Buit as you said at the very beginning,  op->u.get_para.freq_num is validated 
> against pmpt->perf.state_count, so ig the above scenario will not happen, 
> I'll delete this commit.

Not sure yet whether deleting is the right course of action.

Jan