linux-kernel - Re: [PATCH] i2c: drop ancient protection against sysfs refcounting issues

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] [day] [month] [year] [list]

Message-ID: <20150120113508.GA1067@katana>
Date:	Tue, 20 Jan 2015 12:35:08 +0100
From:	Wolfram Sang <wsa@...-dreams.de>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
Cc:	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	linux-mips@...ux-mips.org,
	Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
	linux-kernel@...r.kernel.org, Julia Lawall <julia.lawall@...6.fr>,
	linux-arm-kernel@...ts.infradead.org,
	linuxppc-dev@...ts.ozlabs.org, Jean Delvare <jdelvare@...e.de>
Subject: Re: [PATCH] i2c: drop ancient protection against sysfs refcounting
 issues


> > Right, and I'm not saying it should be, just move the existing logic
> > into the release callback, and the code flow should be the same and we
> > don't end up with an "empty" release callback.

But as Russell says, even if we don't have the empty callback, we still
create the problem shown by DEBUG_KOBJECT_RELEASE which wasn't there
before?

> IMHO there are two possibilities here:
> 
> 1. leave it as-is, where we ensure that the remainder of i2c_del_adapter
>    does not complete until the release callback has been called.
> 
> 2. fix it properly by taking (eg) the netdev approach to i2c_adapter,
>    or an alternative solution which results in decoupling the lifetime
>    of the struct device from the i2c_adapter.
> 
> Either of these would be much better than removing the completion and
> then moving a chunk of code to make it "look" safer than it actually is
> and thereby introducing potential use-after-free bugs.

I agree. As much as I'd love option 2) I don't see that on the horizon.
So, let's keep things as they are. What probably makes sense is to
update the comment with something like this? I took the liberty and used
some wording from Russell:

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index e227dff62a85..1c89a08fae2a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -1778,11 +1778,14 @@ void i2c_del_adapter(struct i2c_adapter *adap)
 	/* device name is gone after device_unregister */
 	dev_dbg(&adap->dev, "adapter [%s] unregistered\n", adap->name);
 
-	/* clean up the sysfs representation */
+	/* wait until all references to the device are gone
+	 *
+	 * FIXME: This is old code and should ideally be replaced by an
+	 * alternative which results in decoupling the lifetime of the struct
+	 * device from the i2c_adapter, like spi or netdev do.
+	 */
 	init_completion(&adap->dev_released);
 	device_unregister(&adap->dev);
-
-	/* wait for sysfs to drop all references */
 	wait_for_completion(&adap->dev_released);
 
 	/* free bus id */


Thanks for all the input, it is very much appreciated!

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)