Re: [PATCH 6/6] ACPI: PCC: Preserve shared memory signature in OpRegion handler

[Rafael J. Wysocki (Intel)]

On Sat, Jun 27, 2026 at 6:39 PM Sudeep Holla <sudeep.holla@xxxxxxxxxx> wrote:
>
> ACPI defines a PCC OperationRegion as the shared memory fields that
> follow the PCC signature. The OperationRegion length is likewise the
> total size of the fields that succeed the signature in shared memory.
>
> ACPI 6.6 adds clarification that the PCC shared memory signature is
> populated by the platform and verified by OSPM.
>
> The PCC address space handler currently copies the OperationRegion
> buffer to and from the start of the PCC shared memory region. That can
> overwrite or expose the signature at byte offset 0, and it also misses
> the last 4 bytes of the actual PCC OperationRegion data.
>
> Offset OperationRegion copies by the size of the signature and reject
> regions that do not fit in the shared memory after that signature.
>
> Cc: "Rafael J. Wysocki" <rafael@xxxxxxxxxx>
> Fixes: 77e2a04745ff ("ACPI: PCC: Implement OperationRegion handler for the PCC Type 3 subtype")
> Signed-off-by: Sudeep Holla <sudeep.holla@xxxxxxxxxx>

Does this depend on anything else in the series or is it independent?

> ---
>  drivers/acpi/acpi_pcc.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/acpi_pcc.c b/drivers/acpi/acpi_pcc.c
> index 438c67189511..9881c9ee293d 100644
> --- a/drivers/acpi/acpi_pcc.c
> +++ b/drivers/acpi/acpi_pcc.c
> @@ -28,6 +28,7 @@
>   * to PCC commands
>   */
>  #define PCC_CMD_WAIT_RETRIES_NUM       500ULL
> +#define PCC_SIGNATURE_SIZE             sizeof(u32)
>
>  struct pcc_data {
>         struct pcc_mbox_chan *pcc_chan;
> @@ -74,6 +75,14 @@ acpi_pcc_address_space_setup(acpi_handle region_handle, u32 function,
>         }
>
>         pcc_chan = data->pcc_chan;
> +       if (pcc_chan->shmem_size < PCC_SIGNATURE_SIZE ||
> +           ctx->length > pcc_chan->shmem_size - PCC_SIGNATURE_SIZE) {
> +               pr_err("PCC channel-%d shared memory is too small.\n",
> +                      ctx->subspace_id);
> +               ret = AE_AML_REGION_LIMIT;
> +               goto err_free_channel;
> +       }
> +
>         if (!pcc_chan->mchan->mbox->txdone_irq) {
>                 pr_err("This channel-%d does not support interrupt.\n",
>                        ctx->subspace_id);
> @@ -97,14 +106,17 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
>                                u32 bits, acpi_integer *value,
>                                void *handler_context, void *region_context)
>  {
> -       int ret;
>         struct pcc_data *data = region_context;
> +       void __iomem *pcc_opregion;
>         u64 usecs_lat;
> +       int ret;
> +
> +       pcc_opregion = data->pcc_chan->shmem + PCC_SIGNATURE_SIZE;
>
>         reinit_completion(&data->done);
>
> -       /* Write to Shared Memory */
> -       memcpy_toio(data->pcc_chan->shmem, (void *)value, data->ctx.length);
> +       /* Write to the PCC OperationRegion after the shared memory signature. */
> +       memcpy_toio(pcc_opregion, (void *)value, data->ctx.length);
>
>         ret = mbox_send_message(data->pcc_chan->mchan, NULL);
>         if (ret < 0)
> @@ -125,7 +137,7 @@ acpi_pcc_address_space_handler(u32 function, acpi_physical_address addr,
>
>         mbox_chan_txdone(data->pcc_chan->mchan, ret);
>
> -       memcpy_fromio(value, data->pcc_chan->shmem, data->ctx.length);
> +       memcpy_fromio(value, pcc_opregion, data->ctx.length);
>
>         return AE_OK;
>  }
>
> --
> 2.43.0
>