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

[Zhongqiu Han]

On 3/30/2026 8:55 PM, Zhongqiu Han wrote:
> 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;

Now that the patch has been picked on queue, if the approach in the
current draft looks reasonable, I'm happy to send it out as a proper
fixup.

--
Thx and BRs,
Zhongqiu Han