Re: [PATCH v2 2/2] cpufreq: qcom: Add cpufreq scaling support for Qualcomm Shikra SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Calling kfree() on an interior pointer of a devm-managed array causes immediate slab corruption.
- [High] Delayed work is cancelled before the threaded IRQ is disabled in `qcom_cpufreq_hw_cpu_offline()`, allowing work to be rescheduled after cancellation and causing a Use-After-Free.
- [High] Missing bounds check on `index` parsed from device tree leads to an out-of-bounds array access.
--

commit f331397a70453ca14a6a369d3ebce1fe9043850e
Author: Taniya Das <taniya.das@oss.qualcomm.com>

cpufreq: qcom: Add cpufreq scaling support for Qualcomm Shikra SoC
    
The Qualcomm Shikra cpufreq hardware is functionally identical to EPSS,
but supports only up to 12 frequency lookup table (LUT) entries. Hence,
add shikra_epss_soc_data that reuses EPSS configuration with appropriate
LUT entries limit.

> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index ea9a20d27b8fd..d50b868dced30 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>   */
>  
>  #include <linux/bitfield.h>

[ ... ]

> @@ -416,11 +419,25 @@ static const struct qcom_cpufreq_soc_data epss_soc_data = {
>  	.reg_intr_clr = 0x308,
>  	.reg_perf_state = 0x320,
>  	.lut_row_size = 4,
> +	.lut_max_entries = LUT_MAX_ENTRIES,
> +};
> +
> +static const struct qcom_cpufreq_soc_data shikra_epss_soc_data = {
> +	.reg_enable = 0x0,
> +	.reg_domain_state = 0x20,
> +	.reg_dcvs_ctrl = 0xb0,
> +	.reg_freq_lut = 0x100,
> +	.reg_volt_lut = 0x200,
> +	.reg_intr_clr = 0x308,
> +	.reg_perf_state = 0x320,
> +	.lut_row_size = 4,
> +	.lut_max_entries = 12,
>  };

[Severity: Critical]
This is a pre-existing issue, but calling kfree() on an interior pointer of a
devm-managed array causes immediate slab corruption.

In qcom_cpufreq_hw_driver_probe(), the global array qcom_cpufreq.data is
allocated as a single contiguous block using devm_kzalloc():

    qcom_cpufreq.data = devm_kzalloc(dev, sizeof(struct qcom_cpufreq_data) *
                                     num_domains, GFP_KERNEL);

In qcom_cpufreq_hw_cpu_init(), a CPU policy's driver data is set to point to a
specific element inside this array:

    data = &qcom_cpufreq.data[index];

When a policy is torn down, qcom_cpufreq_hw_cpu_exit() is invoked and
explicitly calls kfree(data):

    static void qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
    {
        ...
        kfree(data);
    }

Does calling kfree() on an interior pointer of a devm-managed block violate
slab allocator invariants, and guarantee a kernel panic followed by a
double-free when devres cleans up the block?

[Severity: High]
This is a pre-existing issue, but delayed work is cancelled before the threaded
IRQ is disabled in qcom_cpufreq_hw_cpu_offline(), allowing work to be
rescheduled after cancellation and causing a use-after-free.

In qcom_cpufreq_hw_cpu_offline(), cancel_delayed_work_sync() is called prior to
disable_irq_nosync():

    cancel_delayed_work_sync(&data->throttle_work);
    irq_set_affinity_and_hint(data->throttle_irq, NULL);
    disable_irq_nosync(data->throttle_irq);

The LMh IRQ is registered as a threaded interrupt (IRQF_ONESHOT). If the
threaded handler qcom_lmh_dcvs_handle_irq() runs concurrently with the offline
process, cancel_delayed_work_sync() can return before the handler executes
schedule_delayed_work().

Because disable_irq_nosync() masks the interrupt but does not wait for the
currently executing threaded handler to finish, the handler can successfully
re-queue the delayed work. 

Since qcom_cpufreq_hw_cpu_exit() does not cancel the delayed work before
destroying resources, will the work fire after the driver is unbound, leading
to a use-after-free and the execution of unloaded module code?

[Severity: High]
This is a pre-existing issue, but a missing bounds check on the index parsed
from the device tree leads to an out-of-bounds array access.

In qcom_cpufreq_hw_cpu_init(), the driver parses the qcom,freq-domain property
from the device tree to obtain an index:

    ret = of_parse_phandle_with_args(cpu_np, "qcom,freq-domain",
                                     "#freq-domain-cells", 0, &args);
    ...
    index = args.args[0];

It immediately uses this index to access the dynamically allocated global
array:

    data = &qcom_cpufreq.data[index];

There is no verification that index is strictly less than num_domains (which is
determined during probe by counting available MMIO resources). 

Could a malformed or malicious device tree supplying an out-of-bounds index
cause the driver to read garbage memory for data->base, leading to a page
fault and kernel panic when it subsequently attempts
readl_relaxed(data->base + ...)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519-shikra-cpufreq-scaling-v2-0-2b09f0e1c9d8@oss.qualcomm.com?part=2