Re: [PATCH v8 2/2] cpufreq: Add boost_freq_req QoS request

[Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>]

On 3/30/2026 3:16 PM, zhenglifeng (A) wrote:
> On 3/30/2026 12:00 PM, Zhongqiu Han wrote:
>> On 3/30/2026 10:10 AM, zhenglifeng (A) wrote:
>>> On 3/29/2026 5:00 PM, Zhongqiu Han wrote:
>>>>> @@ -1377,6 +1386,7 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
>>>>>         }
>>>>>           freq_qos_remove_request(policy->min_freq_req);
>>>>> +    freq_qos_remove_request(policy->boost_freq_req);
>>>>>         kfree(policy->min_freq_req);
>>>>>           cpufreq_policy_put_kobj(policy);
>>>>> @@ -1445,26 +1455,38 @@ static int cpufreq_policy_online(struct cpufreq_policy *policy,
>>>>>         cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
>>>>>           if (new_policy) {
>>>>> +        unsigned int count;
>>>>> +
>>>>>             for_each_cpu(j, policy->related_cpus) {
>>>>>                 per_cpu(cpufreq_cpu_data, j) = policy;
>>>>>                 add_cpu_dev_symlink(policy, j, get_cpu_device(j));
>>>>>             }
>>>>>     -        policy->min_freq_req = kzalloc(2 * sizeof(*policy->min_freq_req),
>>>>> +        count = policy->boost_supported ? 3 : 2;
>>>>> +        policy->min_freq_req = kzalloc(count * sizeof(*policy->min_freq_req),
>>>>>                                GFP_KERNEL);
>>>>>             if (!policy->min_freq_req) {
>>>>>                 ret = -ENOMEM;
>>>>>                 goto out_destroy_policy;
>>>>>             }
>>>>>     +        if (policy->boost_supported) {
>>>>> +            policy->boost_freq_req = policy->min_freq_req + 2;
>>>>> +
>>>>> +            ret = freq_qos_add_request(&policy->constraints,
>>>>> +                           policy->boost_freq_req,
>>>>> +                           FREQ_QOS_MAX,
>>>>> +                           policy->cpuinfo.max_freq);
>>>>> +            if (ret < 0) {
>>>>> +                policy->boost_freq_req = NULL;
>>>>> +                goto out_destroy_policy;
>>>>> +            }
>>>>> +        }
>>>>> +
>>>>>             ret = freq_qos_add_request(&policy->constraints,
>>>>>                            policy->min_freq_req, FREQ_QOS_MIN,
>>>>>                            FREQ_QOS_MIN_DEFAULT_VALUE);
>>>>>             if (ret < 0) {
>>>>> -            /*
>>>>> -             * So we don't call freq_qos_remove_request() for an
>>>>> -             * uninitialized request.
>>>>> -             */
>>>>>                 kfree(policy->min_freq_req);
>>>>>                 policy->min_freq_req = NULL;
>>>>>                 goto out_destroy_policy;
>>>>
>>>> Hi Pierre, Viresh,
>>>>
>>>> Sorry for the late follow-up on v8. While re-reading the patch, I
>>>> noticed a potential UAF issue on an error path — I might be missing
>>>> something, so I'd appreciate a double-check.
>>>>
>>>> min_freq_req, max_freq_req and boost_freq_req all point into the same
>>>> contiguous kzalloc'd block:
>>>>
>>>> slot0 (min_freq_req + 0) -> min_freq_req
>>>> slot1 (min_freq_req + 1) -> max_freq_req
>>>> slot2 (min_freq_req + 2) -> boost_freq_req
>>>>
>>>> If boost_freq_req is successfully added to the QoS constraints list, but
>>>> the subsequent freq_qos_add_request() for min_freq_req fails, the error
>>>> path does:
>>>>
>>>> kfree(policy->min_freq_req); /* frees the entire block, including slot2
>>>> */
>>>> policy->min_freq_req = NULL;
>>>> goto out_destroy_policy;
>>>>
>>>> policy->boost_freq_req is not set to NULL here, so it becomes a dangling
>>>> pointer into freed memory.
>>>> cpufreq_policy_free() is then called from cpufreq_online() and does:
>>>>
>>>> freq_qos_remove_request(policy->boost_freq_req); /* UAF */
>>>> or this boost qos req will leak.
>>>>
>>>
>>> Good catch!
>>>
>>> How about remove the kfree() here and just leave it to
>>> cpufreq_policy_free()?
>>>
>>
>> Thanks for the suggestion — this is another fix approach we can
>> explore, but there seems to be a small caveat.
>>
>> Some additional changes would still be needed; otherwise, removing the
>> kfree() here and deferring it to cpufreq_policy_free() can lead to a
>> warning.
>>
>> The reason is that we neither free policy->min_freq_req nor set policy
>> ->min_freq_req = NULL. As a result, when cpufreq_policy_free() later
>> calls freq_qos_remove_request(policy->min_freq_req), it hits the
>> following warning:
>>
>> if (WARN(!freq_qos_request_active(req),
>>      "%s() called for unknown object\n", __func__))
>>      return -EINVAL;
>>
> 
> Therefore, it seems the only option is to allocate memory separately for
> boost_freq_req.
> 

Thanks Lifeng. Allocating memory separately could also be a direction we
can explore. I also sketched another small example in a separate mail
thread for discussion.


-- 
Thx and BRs,
Zhongqiu Han