Re: [PATCH 15/23] mfd: tps6586: use platform_device_set_of_node()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cleanup of platform sub-devices on the probe error path leads to a resource leak and potential Use-After-Free.
- [High] Missing irq_domain_remove() and irq_free_descs() on error paths and module removal.
--

commit 77ea296910871ac50ea42e73a55af867803293eb
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

mfd: tps6586: use platform_device_set_of_node()

Ahead of reworking the reference counting logic for platform devices,
encapsulate the assignment of the OF node for dynamically allocated
platform devices with the provided helper.

> diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
> index f5f8054466033..3cfd2f02b62f3 100644
> --- a/drivers/mfd/tps6586x.c
> +++ b/drivers/mfd/tps6586x.c
> @@ -397,7 +397,7 @@ static int tps6586x_add_subdevs(struct tps6586x *tps6586x,
>  
>  		pdev->dev.parent = tps6586x->dev;
>  		pdev->dev.platform_data = subdev->platform_data;
> -		pdev->dev.of_node = of_node_get(subdev->of_node);
> +		platform_device_set_of_node(pdev, subdev->of_node);
>  
>  		ret = platform_device_add(pdev);
>  		if (ret) {
>  			platform_device_put(pdev);
>  			goto failed;
>  		}

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path
leak the platform sub-devices?

In tps6586x_i2c_probe(), sub-devices are registered via
tps6586x_add_subdevs(). If a subsequent operation like
devm_register_power_off_handler() fails, the code jumps to err_add_devs and
cleans up MFD devices via mfd_remove_devices():

drivers/mfd/tps6586x.c:tps6586x_i2c_probe() {
    ...
err_add_devs:
	mfd_remove_devices(tps6586x->dev);
err_mfd_add:
	if (client->irq)
		free_irq(client->irq, tps6586x);
	return ret;
}

However, it fails to call tps6586x_remove_subdevs(). The registered
platform sub-devices remain attached, while the parent driver's private
data (tps6586x) is freed.

If any of the leaked sub-devices are subsequently probed or accessed, would
they attempt to access the freed parent driver data, triggering a
use-after-free?

[Severity: High]
This is a pre-existing issue, but does tps6586x_irq_init() leak the IRQ
domain on error paths and module removal?

In tps6586x_irq_init(), an IRQ domain is created using
irq_domain_create_simple(), with the driver's private data (tps6586x) set
as host_data:

drivers/mfd/tps6586x.c:tps6586x_irq_init() {
    ...
	tps6586x->irq_domain = irq_domain_create_simple(dev_fwnode(tps6586x->dev), irq_num,
							new_irq_base, &tps6586x_domain_ops,
							tps6586x);
    ...
}

However, there are no corresponding calls to irq_domain_remove() or
irq_free_descs() on the probe error paths or in tps6586x_i2c_remove().

On unbind or probe failure, the IRQ domain is leaked while the devm_kzalloc
allocated tps6586x structure is freed. If the device is rebound or its
interrupts are looked up, the system may match the leaked IRQ domain on the
device's fwnode.

Would subsequent IRQ mappings invoke tps6586x_irq_map(), dereferencing the
dangling host_data pointer and causing a use-after-free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260521-pdev-fwnode-ref-v1-0-88c324a1b8d2@oss.qualcomm.com?part=15