Re: [PATCH AUTOSEL 6.18-5.10] firmware: imx: scu: manage mailbox channels and global handle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] firmware: imx: scu: missing irq channel cleanup in imx_scu_probe()
- [High] firmware: imx: scu: unmanaged soc_device in imx_scu_soc_init()
--

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

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

This commit registers devres actions in imx_scu_probe() to release
requested mailbox channels and clear imx_sc_ipc_handle on probe failure,
and depopulates child devices if devm_of_platform_populate() fails.

> diff --git a/drivers/firmware/imx/imx-scu.c b/drivers/firmware/imx/imx-scu.c
> index 8c28e25ddc8a6..28d745ae06067 100644
> --- a/drivers/firmware/imx/imx-scu.c
> +++ b/drivers/firmware/imx/imx-scu.c
[ ... ]
> @@ -328,6 +344,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;
>  
>  	ret = imx_scu_soc_init(dev);
>  	if (ret)

[Severity: High]
If devm_of_platform_populate(dev) fails later in imx_scu_probe(), does
imx_scu_soc_init(dev) leave behind an unmanaged soc_device whose attributes
point to freed devm memory?

In drivers/firmware/imx/imx-scu-soc.c, imx_scu_soc_init() caches sc_ipc in
imx_sc_soc_ipc_handle, allocates soc_dev_attr, soc_dev_attr->revision, and
soc_dev_attr->serial_number using devm on the SCU device, and registers the
SoC device with the unmanaged soc_device_register():

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

	soc_dev_attr = devm_kzalloc(dev, sizeof(*soc_dev_attr),
				    GFP_KERNEL);
	...
	soc_dev_attr->revision = devm_kasprintf(dev, GFP_KERNEL, "%d.%d",
						(val >> 4) & 0xf, val & 0xf);
	...
	soc_dev_attr->serial_number = devm_kasprintf(dev, GFP_KERNEL,
						     "%016llX", uid);
	...
	soc_dev = soc_device_register(soc_dev_attr);
}

In drivers/base/soc.c, soc_device_register() stores the soc_dev_attr pointer
directly in soc_dev->attr.

When devm_of_platform_populate(dev) fails, devres_release_all(dev) frees
soc_dev_attr, revision, serial_number, and sc_ipc while soc_dev remains
registered on soc_bus_type and imx_sc_soc_ipc_handle stays set to the freed
sc_ipc. Reading /sys/bus/soc/devices/socX/* via soc_info_show() or calling
soc_device_match() will then dereference freed memory.

Should imx_scu_soc_init() also register devres cleanup to call
soc_device_unregister(soc_dev) and clear imx_sc_soc_ipc_handle?

> @@ -340,7 +359,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);
> +
> +	return ret;
>  }

[Severity: High]
Similarly, when devm_of_platform_populate(dev) fails here after
imx_scu_enable_general_irq_channel(dev) has succeeded, doesn't the
unmanaged state in drivers/firmware/imx/imx-scu-irq.c remain active while
its devm-allocated backing structures are freed?

Before devm_of_platform_populate(dev) is called, imx_scu_probe() invokes
imx_scu_enable_general_irq_channel(dev):

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

	cl = devm_kzalloc(dev, sizeof(*cl), GFP_KERNEL);
	if (!cl)
		return -ENOMEM;

	cl->dev = dev;
	cl->rx_callback = imx_scu_irq_callback;

	/* SCU general IRQ uses general interrupt channel 3 */
	ch = mbox_request_channel_byname(cl, "gip3");
	...
	INIT_WORK(&imx_sc_irq_work, imx_scu_irq_work_handler);

	if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
				       "#mbox-cells", 0, &spec))
		i = of_alias_get_id(spec.np, "mu");
	...
	wakeup_obj = kobject_create_and_add("scu_wakeup_source", firmware_kobj);
	...
	ret = sysfs_create_file(wakeup_obj, &wakeup_source_attr.attr);
}

If devm_of_platform_populate(dev) fails, devres frees cl and sc_ipc and
clears imx_sc_ipc_handle, but nothing tears down the state created in
imx_scu_enable_general_irq_channel():

1. The "gip3" mailbox channel ch is not freed with mbox_free_channel(ch),
   leaving chan->cl pointing to the freed mbox_client cl (which causes a
   use-after-free in imx_scu_irq_callback() on a gip3 interrupt and causes
   __mbox_bind_client() to return -EBUSY on any later probe retry), and
   imx_sc_irq_work is not cancelled.

2. imx_sc_irq_ipc_handle remains non-NULL and points to the freed sc_ipc, so
   callers such as imx_scu_irq_group_enable() pass the
   !imx_sc_irq_ipc_handle check and dereference freed memory in
   imx_scu_call_rpc().

3. wakeup_obj and wakeup_source_attr are leaked under
   /sys/firmware/scu_wakeup_source (in addition to spec.np from
   of_parse_phandle_with_args() never being released with of_node_put()).

Could imx_scu_enable_general_irq_channel() register devres actions to
clean up the gip3 channel, imx_sc_irq_work, imx_sc_irq_ipc_handle, and
wakeup_obj when probe fails?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-392-sashal@kernel.org?part=1