[PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[PATCH 0/6] ACPI/PCC: Preserve platform-populated PCC signatures

[Sudeep Holla]

ACPI PCC shared memory layouts reserve the first dword for the PCC
signature. ACPI specification defines the signature as 0x50434300 ORed
with the PCC subspace ID, and ACPI 6.6 clarify that the signature is
populated by the platform and verified by OSPM.

This series centralizes PCC shared memory signature validation in the PCC
mailbox controller and stops PCC users from rewriting the signature before
each command. Clients that previously copied complete local PCC headers
now update only the mutable command/status/flags/length/payload fields.

The final patch also fixes the PCC OperationRegion handler. ACPI defines
a PCC OperationRegion as the shared memory fields that follow the PCC
signature, with the OperationRegion length covering only those fields. The
handler is updated to copy to/from the region after the signature and to
reject regions that do not fit there.

All patches can go independently as there is no strict dependency between
them and posted together for the complete context.

Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
Sudeep Holla (6):
      mailbox: pcc: Validate shared memory signature on request
      hwmon: xgene: Stop writing PCC shared memory signature
      i2c: xgene-slimpro: Stop writing PCC shared memory signature
      devfreq: hisi_uncore: Preserve PCC shared memory signature
      soc: hisilicon: kunpeng_hccs: Preserve PCC signatures
      ACPI: PCC: Preserve shared memory signature in OpRegion handler

 drivers/acpi/acpi_pcc.c                | 20 ++++++++++++++----
 drivers/devfreq/hisi_uncore_freq.c     | 15 +++++++-------
 drivers/hwmon/xgene-hwmon.c            |  4 ----
 drivers/i2c/busses/i2c-xgene-slimpro.c |  3 ---
 drivers/mailbox/pcc.c                  | 38 +++++++++++++++++++++++++++++-----
 drivers/soc/hisilicon/kunpeng_hccs.c   | 24 ++++++++-------------
 6 files changed, 65 insertions(+), 39 deletions(-)
---
base-commit: 5a66900afbd6b2a063eebad35294038a654de2b0
change-id: 20260627-acpi_pcc_signature-7b70b0633c8f


-- 
Regards,
Sudeep

[PATCH 2/6] hwmon: xgene: Stop writing PCC shared memory signature

[Sudeep Holla]

ACPI specification defines the generic PCC shared memory signature as
the PCC base signature ORed with the subspace ID.

ACPI 6.6 added clarification that the signature is populated by the
platform and verified by OSPM.

The PCC mailbox controller now validates the signature when the channel
is requested. Stop rewriting the signature before each command and
leave the platform-populated value intact.

Cc: Guenter Roeck <linux@roeck-us.net>
Cc: linux-hwmon@vger.kernel.org
Signed-off-by: Sudeep Holla <sudeep.holla@kernel.org>
---
 drivers/hwmon/xgene-hwmon.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
index 11c5d80428cd..38b140c23c88 100644
--- a/drivers/hwmon/xgene-hwmon.c
+++ b/drivers/hwmon/xgene-hwmon.c
@@ -133,10 +133,6 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
 	init_completion(&ctx->rd_complete);
 	ctx->resp_pending = true;
 
-	/* Write signature for subspace */
-	WRITE_ONCE(generic_comm_base->signature,
-		   cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));
-
 	/* Write to the shared command region */
 	WRITE_ONCE(generic_comm_base->command,
 		   cpu_to_le16(MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));

-- 
2.43.0

Re: [PATCH 2/6] hwmon: xgene: Stop writing PCC shared memory signature

[sashiko-bot]

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

Pre-existing issues:
- [High] Use-After-Free and Race Condition in Module Teardown
- [High] Asynchronous Alarms Erroneously Intercepted as Synchronous Responses
- [High] Endianness Corruption and Direct I/O Memory Dereference
- [High] Hardware State Machine Race via Improper PCC Memory Ordering
--

commit b2acc53f504dcc4234ad2b3494e6b67787941414
Author: Sudeep Holla <sudeep.holla@kernel.org>
Subject: hwmon: xgene: Stop writing PCC shared memory signature

This commit stops rewriting the generic PCC shared memory signature before each
command in xgene_hwmon_pcc_rd(). This ensures the platform-populated signature
is preserved as per ACPI 6.6 clarifications.

> diff --git a/drivers/hwmon/xgene-hwmon.c b/drivers/hwmon/xgene-hwmon.c
> index 11c5d80428cdf..38b140c23c88e 100644
> --- a/drivers/hwmon/xgene-hwmon.c
> +++ b/drivers/hwmon/xgene-hwmon.c
> @@ -133,10 +133,6 @@ static int xgene_hwmon_pcc_rd(struct xgene_hwmon_dev *ctx, u32 *msg)
>  	init_completion(&ctx->rd_complete);
>  	ctx->resp_pending = true;
>  
> -	/* Write signature for subspace */
> -	WRITE_ONCE(generic_comm_base->signature,
> -		   cpu_to_le32(PCC_SIGNATURE | ctx->mbox_idx));
> -
>  	/* Write to the shared command region */
>  	WRITE_ONCE(generic_comm_base->command,
>  		   cpu_to_le16(MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));

While reviewing this patch, I noticed a few pre-existing issues in this file.

drivers/hwmon/xgene-hwmon.c:xgene_hwmon_pcc_rd()

	/* Write to the shared command region */
	WRITE_ONCE(generic_comm_base->command,
		   cpu_to_le16(MSG_TYPE(msg[0]) | PCC_CMD_GENERATE_DB_INTR));

	/* Flip CMD COMPLETE bit */
	val = le16_to_cpu(READ_ONCE(generic_comm_base->status));
	val &= ~PCC_STATUS_CMD_COMPLETE;
	WRITE_ONCE(generic_comm_base->status, cpu_to_le16(val));

	/* Copy the message to the PCC comm space */
	for (i = 0; i < sizeof(struct slimpro_resp_msg) / 4; i++)
		WRITE_ONCE(ptr[i], cpu_to_le32(msg[i]));

[Severity: High]
This isn't a bug introduced by this patch, but does this ordering violate the
ACPI PCC specification?

It appears we write the command register and clear the PCC_STATUS_CMD_COMPLETE
bit before actually writing the payload to the PCC communication space in the
loop.

Could this allow the platform management processor, if it's polling the status
bit, to observe the command as ready and execute it using stale or partially
written payload data before the loop finishes?


drivers/hwmon/xgene-hwmon.c:xgene_hwmon_pcc_rx_cb()

	struct acpi_pcct_shared_memory __iomem *generic_comm_base =
							ctx->pcc_chan->shmem;
...
	msg = generic_comm_base + 1;
...
			ctx->sync_msg.msg = ((u32 *)msg)[0];
			ctx->sync_msg.param1 = ((u32 *)msg)[1];
			ctx->sync_msg.param2 = ((u32 *)msg)[2];

[Severity: High]
This is a pre-existing issue, but is it safe to directly cast and dereference
the __iomem pointer generic_comm_base + 1 as a native u32 *?

The PCC shared memory specification relies on little-endian encoding. Since
X-Gene supports big-endian architectures, wouldn't directly dereferencing the
native pointer cause the fields to be byte-swapped on big-endian systems,
potentially failing all MSG_TYPE checks and causing synchronous commands to
unconditionally timeout?


drivers/hwmon/xgene-hwmon.c:xgene_hwmon_rx_cb()

	/* Check for sync query */
	if (ctx->resp_pending &&
	    ((MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_ERR) ||
	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_DBG &&
	      MSG_SUBTYPE(((u32 *)msg)[0]) == DBG_SUBTYPE_SENSOR_READ) ||
	     (MSG_TYPE(((u32 *)msg)[0]) == MSG_TYPE_PWRMGMT &&
	      MSG_SUBTYPE(((u32 *)msg)[0]) == PWRMGMT_SUBTYPE_TPC &&
	      TPC_CMD(((u32 *)msg)[0]) == TPC_ALARM))) {
		/* Check if platform completes command */
...
			ctx->sync_msg.msg = ((u32 *)msg)[0];
...
			/* Operation waiting for response */
			complete(&ctx->rd_complete);
			return;

[Severity: High]
This is a pre-existing issue, but does this conditional block improperly
intercept asynchronous alarms?

If a thermal alarm (TPC_ALARM) fires while a lengthy synchronous sensor read
is pending (ctx->resp_pending is true), it looks like the driver would match
the TPC_ALARM event, complete the read with the alarm data, and return early.

Wouldn't this cause the async alarm to be silently lost instead of being
routed to the async workqueue, while simultaneously causing the active sensor
reading to evaluate to corrupted values? This identical logic appears in
both xgene_hwmon_pcc_rx_cb() and xgene_hwmon_rx_cb().


drivers/hwmon/xgene-hwmon.c:xgene_hwmon_remove()

static void xgene_hwmon_remove(struct platform_device *pdev)
{
	struct xgene_hwmon_dev *ctx = platform_get_drvdata(pdev);

	cancel_work_sync(&ctx->workq);
	hwmon_device_unregister(ctx->hwmon_dev);
	kfifo_free(&ctx->async_msg_fifo);
	if (acpi_disabled)
		mbox_free_channel(ctx->mbox_chan);
	else
		pcc_mbox_free_channel(ctx->pcc_chan);
}

[Severity: High]
This is a pre-existing issue, but is there a race condition during module
teardown here?

We call kfifo_free(&ctx->async_msg_fifo) and cancel_work_sync(&ctx->workq)
before disabling the hardware mailbox interrupts with mbox_free_channel() or
pcc_mbox_free_channel().

If a late hardware interrupt arrives after the structures are freed but before
the channel is disabled, wouldn't the RX callback (xgene_hwmon_rx_cb or
xgene_hwmon_pcc_rx_cb) enqueue data into the freed async_msg_fifo and
schedule work on the canceled workq, leading to a use-after-free and
potential crash?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260627-acpi_pcc_signature-v1-0-c1b7268d4fdc@kernel.org?part=2