Re: [PATCH] PCI/pwrctrl: tc9563: take i2c adapter module reference

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on `port` index allows out-of-bounds array access on malformed device tree.
- [High] Missing device link to I2C adapter leads to Use-After-Free if the adapter is unbound.
- [Medium] Unconditional calls to `tc9563_pwrctrl_power_off()` cause regulator usage count underflows.
--

--- Patch [1]: [PATCH] PCI/pwrctrl: tc9563: take i2c adapter module reference ---
commit ef55bbdfd2a346b4d8f7147c1d7a0c1ab090cb03
Author: Johan Hovold <johan@kernel.org>
Subject: PCI/pwrctrl: tc9563: take i2c adapter module reference

This commit switches to using of_get_i2c_adapter_by_node() to take a module
reference to the I2C adapter. This prevents the I2C subsystem from blocking
uninterruptibly during adapter deregistration when consumers are holding a
reference, allowing module unloads to fail gracefully instead.

> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 488e1ec34a7fb..83ae9f256b69e 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -549,7 +549,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, ret, "Failed to read i2c-parent property\n");
>  
>  	i2c_node = of_parse_phandle(dev->of_node, "i2c-parent", 0);
> -	tc9563->adapter = of_find_i2c_adapter_by_node(i2c_node);
> +	tc9563->adapter = of_get_i2c_adapter_by_node(i2c_node);
>  	of_node_put(i2c_node);
>  	if (!tc9563->adapter)
>  		return dev_err_probe(dev, -EPROBE_DEFER, "Failed to find I2C adapter\n");
> @@ -557,7 +557,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  	tc9563->client = i2c_new_dummy_device(tc9563->adapter, addr);

[Severity: High]
This is a pre-existing issue, but does this code leave the platform device
vulnerable to a use-after-free?

If a privileged user unbinds the I2C adapter via sysfs, the I2C core
unregisters and frees all child clients, including this dummy client. Since
no device link is created (such as with DL_FLAG_AUTOREMOVE_CONSUMER), the
platform device remains bound and unaware, holding a dangling pointer to the
freed client.

Could subsequent operations like device removal or PCI power callbacks
dereference this freed client?

[Severity: High]
This is also a pre-existing issue, but could a malformed device tree cause an
out-of-bounds access on the configuration array?

Looking further down in tc9563_pwrctrl_probe(), the port variable is
incremented for each child node without bounds checking:

	for_each_child_of_node_scoped(node, child) {
		port++;
		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
		...
	}

If the device tree provides more child nodes than expected, does the port
variable increment beyond TC9563_MAX?

When passed to tc9563_pwrctrl_parse_device_dt(), this unvalidated index is
directly used to access the statically sized array:

	struct tc9563_pwrctrl_cfg *cfg = &tc9563->cfg[port];

Could this lead to memory corruption if the loop exceeds the array bounds?

>  	if (IS_ERR(tc9563->client)) {
>  		dev_err(dev, "Failed to create I2C client\n");
> -		put_device(&tc9563->adapter->dev);
> +		i2c_put_adapter(tc9563->adapter);
>  		return PTR_ERR(tc9563->client);
>  	}

[ ... ]

> @@ -625,7 +625,7 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>  	tc9563_pwrctrl_power_off(&tc9563->pwrctrl);

[Severity: Medium]
This is a pre-existing issue, but could this unconditional call to power off
cause a regulator usage count underflow?

Because the regulators are only enabled lazily when the PCI core invokes
the .power_on callback, they may not have been enabled if
devm_pci_pwrctrl_device_set_ready() fails during probe.

>  remove_i2c:
>  	i2c_unregister_device(tc9563->client);
> -	put_device(&tc9563->adapter->dev);
> +	i2c_put_adapter(tc9563->adapter);
>  	return ret;
>  }
>  
> @@ -637,7 +637,7 @@ static void tc9563_pwrctrl_remove(struct platform_device *pdev)
>  
>  	tc9563_pwrctrl_power_off(&tc9563->pwrctrl);

[Severity: Medium]
Similarly, this is a pre-existing issue, but if the device is removed before
being explicitly powered on by the PCI core, will this unconditional power
off also trigger a regulator usage count underflow during removal?

>  	i2c_unregister_device(tc9563->client);
> -	put_device(&tc9563->adapter->dev);
> +	i2c_put_adapter(tc9563->adapter);
>  }
>  
>  static const struct of_device_id tc9563_pwrctrl_of_match[] = {

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