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

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

On 3/30/2026 1:20 PM, Viresh Kumar wrote:
> On 29-03-26, 17:00, Zhongqiu Han wrote:
>> 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.
> 
> Nice catch.
> 
> The right solution to this I guess is to do kfree and setting min_freq_req to
> NULL if boost_freq_req fails (just like what happens in min_freq_req failure
> now) and then for later failures, don't do kfree at all but just set the failed
> qos feature to NULL (like what is done for max_freq_req now).
> 


Thanks Viresh — agreed, that approach makes sense.
I sketched a small example along those lines for discussion only if
needed: add boost_freq_req early when boost_supported, free the shared
allocation if that add fails, and on later failures just unwind without
freeing the block.


+ 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;
+                 kfree(policy->min_freq_req);
+                 policy->min_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.
-          */
+         if (policy->boost_freq_req) {
+                 freq_qos_remove_request(policy->boost_freq_req);
+                 policy->boost_freq_req = NULL;
+         }
           kfree(policy->min_freq_req);
           policy->min_freq_req = NULL;
           goto out_destroy_policy;



-- 
Thx and BRs,
Zhongqiu Han