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

[Zhongqiu Han]

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