diff mbox series

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

Message ID 20260618143727.18534-1-udaykhare77@gmail.com (mailing list archive)
State New
Headers
Series [v3] i2c: sun6i-p2wi: Fix device node reference leak in p2wi_probe |

Commit Message

Uday Khare June 18, 2026, 2:37 p.m. UTC 
In p2wi_probe(), the device node reference obtained via
of_get_next_available_child() is stored in childnp. This reference is
never released, causing a device node reference leak.

Fix this by calling of_node_put(childnp) on both the error and success
paths.

Fixes: 3e833490fae5 ("i2c: sunxi: add P2WI (Push/Pull 2 Wire Interface) controller support")
Signed-off-by: Uday Khare <udaykhare77@gmail.com>
---
v3:
- Revert back to manual of_node_put() because the function uses goto-based
  error handling, and mixing the two styles is discouraged (reported by
  Sashiko-bot).
v2:
- Use __free(device_node) and include <linux/cleanup.h> to automate the device
  node reference cleanup instead of manually calling of_node_put() on error and
  success paths (suggested by Chen-Yu Tsai).

 drivers/i2c/busses/i2c-sun6i-p2wi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

sashiko-bot@kernel.org June 18, 2026, 2:44 p.m. UTC | #1 
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?
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-sun6i-p2wi.c b/drivers/i2c/busses/i2c-sun6i-p2wi.c
index dffbe776a195..7c0239f8937e 100644
--- a/drivers/i2c/busses/i2c-sun6i-p2wi.c
+++ b/drivers/i2c/busses/i2c-sun6i-p2wi.c
@@ -184,7 +184,6 @@  static int p2wi_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-	struct device_node *childnp;
 	unsigned long parent_clk_freq;
 	u32 clk_freq = I2C_MAX_STANDARD_MODE_FREQ;
 	struct p2wi *p2wi;
@@ -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);
+		}
 
 		p2wi->target_addr = target_addr;
+		of_node_put(childnp);
 	}
 
 	p2wi->regs = devm_platform_ioremap_resource(pdev, 0);