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

[sashiko-bot@kernel.org]

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