Re: [PATCH v3] i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling `of_node_put(childnp)` before passing `childnp` to `dev_err_probe()` causes a use-after-free because the `%pOF` format specifier dereferences the freed node.

Pre-existing issues:
- [High] Hardware reset is asserted before the I2C adapter is deleted in `p2wi_remove()`, risking hangs and incomplete transfers.
- [High] The I2C transfer function `p2wi_smbus_xfer()` uses `wait_for_completion()` without a timeout, which can permanently hang the kernel task if the hardware fails.
--

commit c8d2fd420d274dc476834daabf9f1fb97be750d0
Author: Uday Khare <udaykhare77@gmail.com>

i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe

This commit addresses a device node reference leak in p2wi_probe() by calling
of_node_put() on the child node reference obtained via
of_get_next_available_child(). The fix is applied to both the success and
error paths.

> diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c
> index dffbe776a195d..7c0239f8937ea 100644
> --- a/drivers/i2c/busses/i2c-sun6i-p2wi.c
> +++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c

[ ... ]

> @@ -217,14 +216,19 @@ static int p2wi_probe(struct platform_device *pdev)
>  	 * In this case the target_addr is set to -1 and won't be checked when
>  	 * launching a P2WI transfer.
>  	 */
> +	struct device_node *childnp;
> +
>  	childnp = of_get_next_available_child(np, NULL);
>  	if (childnp) {
>  		ret = of_property_read_u32(childnp, "reg", &target_addr);
> -		if (ret)
> +		if (ret) {
> +			of_node_put(childnp);
>  			return dev_err_probe(dev, -EINVAL,
>  					     "invalid target address on node %pOF\n", childnp);

[Severity: High]
Does calling of_node_put() before dev_err_probe() cause a use-after-free?

The %pOF format specifier in dev_err_probe() will dereference childnp to read
its full name. Since of_node_put() decrements the reference count and can
potentially free the node, accessing it immediately afterwards might lead to
a use-after-free.

Could the node be released after logging the error instead?

> +		}
>  
>  		p2wi->target_addr = target_addr;
> +		of_node_put(childnp);
>  	}

While reviewing the above, I also noticed a couple of pre-existing issues in
the same file:

drivers/i2c/busses/i2c-sun6i-p2wi.c:p2wi_remove() {
    ...
	reset_control_assert(p2wi->rstc);
	i2c_del_adapter(&p2wi->adapter);
}

[Severity: High]
This is a pre-existing issue, but could asserting the hardware reset before
deleting the I2C adapter risk hanging the system?

If reset_control_assert() disables the hardware while i2c_del_adapter() is
still waiting for ongoing transfers to finish or accepting new ones, any
in-flight transfers might interact with a disabled controller. This could
lead to the driver waiting indefinitely for an interrupt.

Should i2c_del_adapter() be called before asserting the hardware reset?


drivers/i2c/busses/i2c-sun6i-p2wi.c:p2wi_smbus_xfer() {
    ...
	writel(P2WI_CTRL_START_TRANS | P2WI_CTRL_GLOBAL_INT_ENB,
	       p2wi->regs + P2WI_CTRL);

	wait_for_completion(&p2wi->complete);
    ...
}

[Severity: High]
This is a pre-existing issue, but can this wait_for_completion() hang the
kernel task permanently?

If the hardware fails to generate an interrupt due to a bus lockup, a missed
edge, or reset, the calling thread will block indefinitely in an
uninterruptible sleep state because there is no timeout mechanism.

Would it be safer to use wait_for_completion_timeout() and handle the
timeout condition by reporting an error and cleaning up?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260618143727.18534-1-udaykhare77@gmail.com?part=1