Re: [PATCH] cpufreq: pcc-cpufreq: validate the ACPI interface data

[Rafael J. Wysocki (Intel)]

On Tue, Nov 3, 2026 at 12:42 PM Kang Chen <k.chen@xxxxxxxxxxxxxxxx> wrote:
>
> pcc_get_offset() reads elements[0] and elements[1] from the PCCP
> package without checking that the package has two elements.  A
> zero-length package dereferences elements[0] out of bounds, and a
> one-element package reads elements[1] out of bounds.
>
> It also uses the returned 64-bit values as byte offsets into the
> ioremap()ed PCCH region after truncating them to u32, without checking
> the mapping size.  A firmware-provided PCCP method can therefore make
> the driver clear or program an address outside the mapping.
>
> The PCCH and _OSC replies have the same problem: pcc_cpufreq_evaluate()
> reads elements[0..3] of the PCCH package and casts its two buffer
> elements without checking the package count or the buffer lengths, and
> pcc_cpufreq_do_osc() reads two u32 values from an _OSC buffer without
> checking that it is at least eight bytes long.  A short or malformed
> reply makes the driver read past the ACPI allocation.
>
> Require a two-element PCCP package, keep the offsets in u64 while
> validating them against the mapping size, and only then store the u32
> values.  Also validate the PCCH package count, the two resource buffer
> lengths, the minimum PCCH mapping size, and the _OSC buffer length.

If there are no known examples of firmware with the problems mentioned
above, this should be mentioned here.

If there are such examples, pointers to them should be provided.

> Fixes: 0f1d683fb35d ("[CPUFREQ] Processor Clocking Control interface driver")
> Signed-off-by: Kang Chen <k.chen@xxxxxxxxxxxxxxxx>
> ---
>  drivers/cpufreq/pcc-cpufreq.c | 45 ++++++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/pcc-cpufreq.c b/drivers/cpufreq/pcc-cpufreq.c
> index 83e6f73dc9a4..c7c65104fc09 100644
> --- a/drivers/cpufreq/pcc-cpufreq.c
> +++ b/drivers/cpufreq/pcc-cpufreq.c
> @@ -92,6 +92,7 @@ struct pcc_header {
>
>  static void __iomem *pcch_virt_addr;
>  static struct pcc_header __iomem *pcch_hdr;
> +static u64 pcch_size;
>
>  static DEFINE_SPINLOCK(pcc_lock);
>
> @@ -136,6 +137,7 @@ static inline void pcc_clear_mapping(void)
>         if (pcch_virt_addr)
>                 iounmap(pcch_virt_addr);
>         pcch_virt_addr = NULL;
> +       pcch_size = 0;
>  }
>
>  static unsigned int pcc_get_freq(unsigned int cpu)
> @@ -253,6 +255,7 @@ static int pcc_get_offset(int cpu)
>         union acpi_object *pccp, *offset;
>         struct pcc_cpu *pcc_cpu_data;
>         struct acpi_processor *pr;
> +       u64 input_offset, output_offset;
>         int ret = 0;
>
>         pr = per_cpu(processors, cpu);
> @@ -266,7 +269,8 @@ static int pcc_get_offset(int cpu)
>                 return -ENODEV;
>
>         pccp = buffer.pointer;
> -       if (!pccp || pccp->type != ACPI_TYPE_PACKAGE) {
> +       if (!pccp || pccp->type != ACPI_TYPE_PACKAGE ||
> +           pccp->package.count < 2) {
>                 ret = -ENODEV;
>                 goto out_free;
>         }
> @@ -277,7 +281,7 @@ static int pcc_get_offset(int cpu)
>                 goto out_free;
>         }
>
> -       pcc_cpu_data->input_offset = offset->integer.value;
> +       input_offset = offset->integer.value;
>
>         offset = &(pccp->package.elements[1]);
>         if (!offset || offset->type != ACPI_TYPE_INTEGER) {
> @@ -285,7 +289,21 @@ static int pcc_get_offset(int cpu)
>                 goto out_free;
>         }
>
> -       pcc_cpu_data->output_offset = offset->integer.value;
> +       output_offset = offset->integer.value;
> +
> +       /*
> +        * The offsets are used as byte displacements into the PCCH mapping.
> +        * Check them against its size before truncating them to u32.
> +        */
> +       if (input_offset > U32_MAX || output_offset > U32_MAX ||
> +           input_offset + BUF_SZ > pcch_size ||
> +           output_offset + BUF_SZ > pcch_size) {
> +               ret = -ENODEV;
> +               goto out_free;
> +       }
> +
> +       pcc_cpu_data->input_offset = input_offset;
> +       pcc_cpu_data->output_offset = output_offset;
>
>         memset_io((pcch_virt_addr + pcc_cpu_data->input_offset), 0, BUF_SZ);
>         memset_io((pcch_virt_addr + pcc_cpu_data->output_offset), 0, BUF_SZ);
> @@ -334,7 +352,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>                 return -ENODEV;
>
>         out_obj = output.pointer;
> -       if (out_obj->type != ACPI_TYPE_BUFFER) {
> +       if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER ||
> +           out_obj->buffer.length < 2 * sizeof(u32)) {
>                 ret = -ENODEV;
>                 goto out_free;
>         }
> @@ -365,7 +384,8 @@ static int __init pcc_cpufreq_do_osc(acpi_handle *handle)
>                 return -ENODEV;
>
>         out_obj = output.pointer;
> -       if (out_obj->type != ACPI_TYPE_BUFFER) {
> +       if (!out_obj || out_obj->type != ACPI_TYPE_BUFFER ||
> +           out_obj->buffer.length < 2 * sizeof(u32)) {
>                 ret = -ENODEV;
>                 goto out_free;
>         }
> @@ -418,13 +438,15 @@ static int __init pcc_cpufreq_evaluate(void)
>                 return -ENODEV;
>
>         out_obj = output.pointer;
> -       if (out_obj->type != ACPI_TYPE_PACKAGE) {
> +       if (!out_obj || out_obj->type != ACPI_TYPE_PACKAGE ||
> +           out_obj->package.count < 4) {
>                 ret = -ENODEV;
>                 goto out_free;
>         }
>
>         member = &out_obj->package.elements[0];
> -       if (member->type != ACPI_TYPE_BUFFER) {
> +       if (member->type != ACPI_TYPE_BUFFER ||
> +           member->buffer.length < sizeof(struct pcc_memory_resource)) {
>                 ret = -ENODEV;
>                 goto out_free;
>         }
> @@ -448,6 +470,11 @@ static int __init pcc_cpufreq_evaluate(void)
>                 goto out_free;
>         }
>
> +       if (mem_resource->address_length < sizeof(struct pcc_header)) {
> +               ret = -ENODEV;
> +               goto out_free;
> +       }
> +
>         pcch_virt_addr = ioremap(mem_resource->minimum,
>                                         mem_resource->address_length);
>         if (pcch_virt_addr == NULL) {
> @@ -455,6 +482,7 @@ static int __init pcc_cpufreq_evaluate(void)
>                 ret = -ENOMEM;
>                 goto out_free;
>         }
> +       pcch_size = mem_resource->address_length;
>         pcch_hdr = pcch_virt_addr;
>
>         pr_debug("probe: PCCH header (virtual) addr: 0x%p\n", pcch_hdr);
> @@ -480,7 +508,8 @@ static int __init pcc_cpufreq_evaluate(void)
>                 ioread32(&pcch_hdr->minimum_frequency));
>
>         member = &out_obj->package.elements[1];
> -       if (member->type != ACPI_TYPE_BUFFER) {
> +       if (member->type != ACPI_TYPE_BUFFER ||
> +           member->buffer.length < sizeof(struct pcc_register_resource)) {
>                 ret = -ENODEV;
>                 goto pcch_free;
>         }
> --
> 2.34.1
>