linux-kernel - Re: [PATCH] i3c: fix refcount inconsistency in i3c_master

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251013210953.GA2124@DESKTOP-2SPVBES.localdomain>
Date: Tue, 14 Oct 2025 05:09:53 +0800
From: Shuhao Fu <sfual@....ust.hk>
To: Frank Li <Frank.li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>,
        linux-i3c@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] i3c: fix refcount inconsistency in i3c_master_register

On Mon, Oct 13, 2025 at 04:19:00PM -0400, Frank Li wrote:
> On Fri, Oct 10, 2025 at 02:34:08PM +0800, Shuhao Fu wrote:
> > On Thu, Oct 09, 2025 at 12:17:11PM -0400, Frank Li wrote:
> > > On Wed, Oct 08, 2025 at 03:27:09PM +0800, Shuhao Fu wrote:
> > > > In `i3c_master_register`, a possible refcount inconsistency has been
> > > > identified, causing possible resource leak.
> > > >
> > > > Function `of_node_get` increases the refcount of `parent->of_node`. If
> > > > function `i3c_bus_init` fails, the function returns immediately without
> > > > a corresponding decrease, resulting in an inconsistent refcounter.
> > > >
> > > > In this patch, an extra goto label is added to ensure the balance of
> > > > refcount when `i3c_bus_init` fails.
> > > >
> > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > > Signed-off-by: Shuhao Fu <sfual@....ust.hk>
> > > > ---
> > > >  drivers/i3c/master.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > index d946db75d..9f4fe98d2 100644
> > > > --- a/drivers/i3c/master.c
> > > > +++ b/drivers/i3c/master.c
> > > > @@ -2885,7 +2885,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > >
> > > >  	ret = i3c_bus_init(i3cbus, master->dev.of_node);
> > > >  	if (ret)
> > > > -		return ret;
> > > > +		goto err_put_of_node;
> > >
> > > I think it'd better to set release function for master dev to release
> > > of_node because of_node_put() also missed at i3c_master_unregister()
> > >
> > > you can refer drivers/base/platform.c
> > >
> > > Frank
> >
> > Do you mean that we should do `of_node_release` in
> > `platform_device_release`, instead of respecting the refcounting via
> > `of_node_put`?
> 
> Sorry, I checked code again.
> 
> static void i3c_masterdev_release(struct device *dev)
> {
>         ...
>         of_node_put(dev->of_node);
> }
> 
> i3c_master_register()
> {
> 	...
> 	master->dev.release = i3c_masterdev_release;
> 	...
> };
> 
> Suppose of_node_put() will be auto called when put_device(&master->dev);
> 
> Do you really meet the problem or just static anaysis?
> 
> Frank

Honestly, it's from static analysis.

My apologies for overlooking the release handle. I checked the code once
again. It still looks suspicious as it would not call `put_device` if it
fails. I also checked call sites related to `i3c_master_register` and
they dont seem to do the clean-up if register fails.

Shuhao
> >
> > >
> > > >
> > > >  	device_initialize(&master->dev);
> > > >  	dev_set_name(&master->dev, "i3c-%d", i3cbus->id);
> > > > @@ -2973,6 +2973,9 @@ int i3c_master_register(struct i3c_master_controller *master,
> > > >  err_put_dev:
> > > >  	put_device(&master->dev);
> > > >
> > > > +err_put_of_node:
> > > > +	of_node_put(master->dev.of_node);
> > > > +
> > > >  	return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > --
> > > > 2.39.5 (Apple Git-154)
> > > >
> > > >
> > > > --
> > > > linux-i3c mailing list
> > > > linux-i3c@...ts.infradead.org
> > > > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7C837a825f1f3443950e6a08de0a95cb5f%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638959835659671475%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=B0ZvY%2FDKW3VdG%2FVamjGUUg%2BVr%2BZbtHIf4otgBKhje1s%3D&reserved=0
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@...ts.infradead.org
> > https://apc01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infradead.org%2Fmailman%2Flistinfo%2Flinux-i3c&data=05%7C02%7Csfual%40connect.ust.hk%7C837a825f1f3443950e6a08de0a95cb5f%7C6c1d415239d044ca88d9b8d6ddca0708%7C1%7C0%7C638959835659696068%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=KpMM%2FsTa6G1o3q%2Bcqx6iTb3VbCDq723lCXgcA9GGetI%3D&reserved=0