Re: [PATCH] arm64: dts: qcom: glymur: use polling mode for SCMI transfers

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

Pre-existing issues:
- [Critical] Dangling pointer in xfer->async_done leads to stack Use-After-Free or NULL pointer dereference on late delayed response.
--

--- Patch [1]: [PATCH] arm64: dts: qcom: glymur: use polling mode for SCMI transfers ---
commit 7c8d066ac6f378514463d4f6eb7b970a95b13e88
Author: Jesse Casco <[email protected]>

arm64: dts: qcom: glymur: use polling mode for SCMI transfers

This commit marks the SCMI transport on qcom glymur to use polling mode. This
works around the CPUCP firmware writing replies to shared memory but failing
to ring the completion doorbell. Using arm,no-completion-irq ensures the
scmi-cpufreq driver successfully probes.

> diff --git a/arch/arm64/boot/dts/qcom/glymur.dtsi b/arch/arm64/boot/dts/qcom/glymur.dtsi
> index f04058d2e0891..b9db81a19d186 100644
> --- a/arch/arm64/boot/dts/qcom/glymur.dtsi
> +++ b/arch/arm64/boot/dts/qcom/glymur.dtsi
> @@ -412,6 +412,12 @@ scmi {
>  			mbox-names = "tx", "rx";
>  			shmem = <&cpu_scp_lpri1>, <&cpu_scp_lpri0>;
>  
> +			/*
> +			 * The CPUCP writes SCMI replies into the shmem but
> +			 * never rings the completion doorbell, so poll.
> +			 */
> +			arm,no-completion-irq;
> +
>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  

[Severity: Critical]
This isn't a bug introduced by this patch, but could a late delayed response
trigger a data race causing a use-after-free on the async_done stack frame or
a NULL pointer dereference?

In drivers/firmware/arm_scmi/driver.c:do_xfer_with_response(), if a delayed
response takes longer than SCMI_MAX_RESPONSE_TIMEOUT,
wait_for_completion_timeout() times out.

The function sets xfer->async_done = NULL without any synchronization and
returns, destroying the async_response stack frame:

drivers/firmware/arm_scmi/driver.c:do_xfer_with_response() {
    ...
	if (!ret) {
		if (!wait_for_completion_timeout(xfer->async_done, timeout)) {
			dev_err(ph->dev,
				"timed out in delayed resp(caller: %pS)\n",
				(void *)_RET_IP_);
			ret = -ETIMEDOUT;
		} else if (xfer->hdr.status) {
    ...
	}

	xfer->async_done = NULL;
    ...
}

However, the transfer remains in the pending hash table until the caller
invokes xfer_put().

If the delayed response interrupt fires in this window, scmi_handle_response()
can successfully acquire the xfer and unconditionally access the unprotected
async_done pointer:

drivers/firmware/arm_scmi/driver.c:scmi_handle_response() {
    ...
	if (xfer->hdr.type == MSG_TYPE_DELAYED_RESP) {
		scmi_clear_channel(info, cinfo);
		complete(xfer->async_done);
		scmi_inc_count(info->dbg, DELAYED_RESPONSE_OK);
	} else {
    ...
}

Could this lead to stack memory corruption if the async_done stack frame is
already destroyed, or a kernel panic if async_done is read after being set to
NULL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1