答复: [外部邮件] Re: [PATCH] cpufreq: acpi-cpufreq: Using cpufreq_for_each_entry() to iterate in extract_io()

[Li,Rongqing]

> > From: Li RongQing <lirongqing@xxxxxxxxx>
> >
> > In extract_io(), the loop iterates up to perf->state_count. However,
> > when building policy->freq_table in acpi_cpufreq_cpu_init(), duplicate
> > frequency entries are skipped, making freq_table smaller than
> > perf->state_count.
> >
> > Iterating perf->state_count times directly over policy->freq_table[i]
> > can result in out-of-bounds array reads. Furthermore,
> > policy->freq_table[i]
> 
> There is no out-of-bounds access. The array has "state_count + 1"
> elements, so every index in "[0, state_count)" is inside the allocation.
> right? The changelog looks like a memory-safety fix, which it is not, and that
> wording alone would get the patch (mis)routed to stable and to CVE bots.
> 
> > does not necessarily correspond to perf->states[i], as the original
> > P-state index is stored in freq_table[entry].driver_data.
> 
> Yes, the real defect is the index space mismatch.
> 
> Might be good to note the side effects of it, AFAICT
> 
> freq_table[i] is not perf->states[i] once any _PSS entry has been skipped. The
> function can therefore return a frequency belonging to a different P-state, or
> 0 (zeroed tail entries), or CPUFREQ_TABLE_END
> (~1u) when "i == valid_states", i.e. 0xfffffffe kHz reported as a frequency. That
> last one is worth spelling out.
> 
> >
> > Fix this by using cpufreq_for_each_entry() to iterate over
> > policy->freq_table, similar to extract_msr().
> >
> 
> Please add one Fixes tag here as well.
> 
> > Signed-off-by: Li RongQing <lirongqing@xxxxxxxxx>
> > ---
> >   drivers/cpufreq/acpi-cpufreq.c | 9 ++++-----
> >   1 file changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/cpufreq/acpi-cpufreq.c
> > b/drivers/cpufreq/acpi-cpufreq.c index 21639d9..87e4923 100644
> > --- a/drivers/cpufreq/acpi-cpufreq.c
> > +++ b/drivers/cpufreq/acpi-cpufreq.c
> > @@ -196,15 +196,14 @@ static int check_amd_hwpstate_cpu(unsigned int
> cpuid)
> >   static unsigned extract_io(struct cpufreq_policy *policy, u32 value)
> >   {
> >   	struct acpi_cpufreq_data *data = policy->driver_data;
> > +	struct cpufreq_frequency_table *pos;
> >   	struct acpi_processor_performance *perf;
> > -	int i;
> >
> >   	perf = to_perf_data(data);
> >
> > -	for (i = 0; i < perf->state_count; i++) {
> > -		if (value == perf->states[i].status)
> > -			return policy->freq_table[i].frequency;
> > -	}
> > +	cpufreq_for_each_entry(pos, policy->freq_table)
> > +		if (value == perf->states[pos->driver_data].status)
> > +			return pos->frequency;
> >   	return 0;
> >   }
> 
> One more potential same issue is in func get_cur_freq_on_cpu()
> 
> cached_freq = policy->freq_table[to_perf_data(data)->state].frequency;
> 
> It is better to fix it as well.
> 
> For v2, please use ./scripts/get_maintainer.pl to generate the CC list so
> linux-kernel@xxxxxxxxxxxxxxx doesn't get missed. Thanks
> 

I will send v2，thanks

[Li,Rongqing] 


> 
> >
> 
> 
> --
> Thx and BRs,
> Zhongqiu Han