Re: [PATCH] firmware: imx: scu: manage mailbox channels and global handle

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Stack memory corruption (Use-After-Free) in IPC message handling due to missing cleanup on timeout. This problem wasn't introduced by this patch.
- [High] Use-After-Free of `gip3` mailbox client due to missing channel cleanup on probe failure. This problem wasn't introduced by this patch, but the patch fails to address it while fixing similar channels.
- [High] Use-After-Free of `soc_dev_attr` due to missing `soc_device_unregister()` on probe failure. This problem wasn't introduced by this patch.
- [High] Dangling secondary global IPC pointers (`imx_sc_irq_ipc_handle`, `imx_sc_soc_ipc_handle`) on probe failure. This problem wasn't introduced by this patch.
- [High] Memory and Sysfs Leak of `wakeup_obj` on probe failure. This problem wasn't introduced by this patch.
--

commit 3f448f232286443e24cf5f0724f9dcaccfa3cf22
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

firmware: imx: scu: manage mailbox channels and global handle

This commit registers devres actions to free mailbox channels and clear the
global handle upon probe failure. It also depopulates partially created child
devices when devm_of_platform_populate() fails.

> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 67b267a7408a1..203aac4212529 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
> @@ -330,6 +346,9 @@ static int imx_scu_probe(struct platform_device *pdev)
>  	init_completion(&sc_ipc->done);
>  
>  	imx_sc_ipc_handle = sc_ipc;
> +	ret = devm_add_action_or_reset(dev, imx_scu_clear_handle, sc_ipc);
> +	if (ret)
> +		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does clearing the primary
handle leave dangling pointers in the secondary ones?

While imx_scu_clear_handle() correctly clears the primary imx_sc_ipc_handle
on failure, imx_scu_enable_general_irq_channel() and imx_scu_soc_init()
have already copied it:

drivers/firmware/imx/imx-scu-soc.c:imx_scu_soc_init() {
    ...
	ret = imx_scu_get_handle(&imx_sc_soc_ipc_handle);
    ...
}

drivers/firmware/imx/imx-scu-irq.c:imx_scu_enable_general_irq_channel() {
    ...
	ret = imx_scu_get_handle(&imx_sc_irq_ipc_handle);
    ...
}

If devm_of_platform_populate() fails, devres frees the sc_ipc object, but
these secondary static pointers remain set. Could this lead to a
Use-After-Free if exported functions like imx_scu_irq_get_status() are
invoked later?

>  
>  	ret = imx_scu_soc_init(dev);
>  	if (ret)

[ ... ]

> @@ -342,7 +361,11 @@ static int imx_scu_probe(struct platform_device *pdev)
>  
>  	dev_info(dev, "NXP i.MX SCU Initialized\n");
>  
> -	return devm_of_platform_populate(dev);
> +	ret = devm_of_platform_populate(dev);
> +	if (ret)
> +		of_platform_depopulate(dev);

[Severity: High]
This is a pre-existing issue, but are there other resources missing devres
cleanup when devm_of_platform_populate() fails?

In imx_scu_enable_general_irq_channel(), the gip3 mailbox channel is
requested, but the pointer is discarded on success without registering a
cleanup action:

drivers/firmware/imx/imx-scu-irq.c:imx_scu_enable_general_irq_channel() {
    ...
	cl = devm_kzalloc(dev, sizeof(*cl), GFP_KERNEL);
    ...
	ch = mbox_request_channel_byname(cl, "gip3");
    ...
}

If probe fails here, the memory for cl is freed by devres, but the mailbox
core still considers the channel active. Could an incoming interrupt trigger
the mailbox RX callback and cause a Use-After-Free?

Additionally, this same function creates a kobject without devres:

	wakeup_obj = kobject_create_and_add("scu_wakeup_source", firmware_kobj);

Does this permanently leak the kobject on failure and break deferred probing
by causing duplicate name errors later?

[Severity: High]
This is a pre-existing issue, but does the SoC device need to be unregistered
if probe fails?

In imx_scu_soc_init(), the SoC device is registered:

drivers/firmware/imx/imx-scu-soc.c:imx_scu_soc_init() {
    ...
	soc_dev_attr = devm_kzalloc(dev, sizeof(*soc_dev_attr), GFP_KERNEL);
    ...
	soc_dev = soc_device_register(soc_dev_attr);
    ...
}

If devm_of_platform_populate() fails, the soc_dev_attr memory is freed by
devres, but the SoC device remains registered. Could reading its sysfs
attributes (like revision or serial_number) dereference the freed memory?

> +
> +	return ret;
>  }

While reviewing the IPC handling, I noticed another pre-existing issue:

[Severity: Critical]
This isn't a bug introduced by this patch, but could a timeout here lead to
stack corruption?

In imx_scu_call_rpc(), if wait_for_completion_timeout() times out, the
function returns immediately:

drivers/firmware/imx/imx-scu.c:imx_scu_call_rpc() {
    ...
	if (!wait_for_completion_timeout(&sc_ipc->done, MAX_RX_TIMEOUT)) {
		dev_err(sc_ipc->dev, "RPC send msg timeout\n");
		mutex_unlock(&sc_ipc->lock);
		return -ETIMEDOUT;
	}
    ...
}

Because sc_ipc->msg isn't set to NULL before returning, it remains pointing
to the caller's stack frame. If a delayed hardware response arrives later,
could imx_scu_rx_callback() write the incoming payload into this reused
stack memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616005902.11634-1-pengpeng@iscas.ac.cn?part=1