* [PATCH] i2c: core: fix NULL pointer dereference under race condition
@ 2016-10-31 19:46 Vladimir Zapolskiy
2016-11-04 19:42 ` Wolfram Sang
0 siblings, 1 reply; 2+ messages in thread
From: Vladimir Zapolskiy @ 2016-10-31 19:46 UTC (permalink / raw)
To: Wolfram Sang, Peter Rosin; +Cc: linux-i2c
Race condition between registering an I2C device driver and
deregistering an I2C adapter device which is assumed to manage that
I2C device may lead to a NULL pointer dereference due to the
uninitialized list head of driver clients.
The root cause of the issue is that the I2C bus may know about the
registered device driver and thus it is matched by bus_for_each_drv(),
but the list of clients is not initialized and commonly it is NULL,
because I2C device drivers define struct i2c_driver as static and
clients field is expected to be initialized by I2C core:
i2c_register_driver() i2c_del_adapter()
driver_register() ...
bus_add_driver() ...
... bus_for_each_drv(..., __process_removed_adapter)
... i2c_do_del_adapter()
... list_for_each_entry_safe(..., &driver->clients, ...)
INIT_LIST_HEAD(&driver->clients);
To solve the problem it is sufficient to do clients list head
initialization before calling driver_register().
The problem was found while using an I2C device driver with a sluggish
registration routine on a bus provided by a physically detachable I2C
master controller, but practically the oops may be reproduced under
the race between arbitraty I2C device driver registration and managing
I2C bus device removal e.g. by unbinding the latter over sysfs:
% echo 21a4000.i2c > /sys/bus/platform/drivers/imx-i2c/unbind
Unable to handle kernel NULL pointer dereference at virtual address 00000000
Internal error: Oops: 17 [#1] SMP ARM
CPU: 2 PID: 533 Comm: sh Not tainted 4.9.0-rc3+ #61
Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
task: e5ada400 task.stack: e4936000
PC is at i2c_do_del_adapter+0x20/0xcc
LR is at __process_removed_adapter+0x14/0x1c
Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
Control: 10c5387d Table: 35bd004a DAC: 00000051
Process sh (pid: 533, stack limit = 0xe4936210)
Stack: (0xe4937d28 to 0xe4938000)
Backtrace:
[<c0667be0>] (i2c_do_del_adapter) from [<c0667cc0>] (__process_removed_adapter+0x14/0x1c)
[<c0667cac>] (__process_removed_adapter) from [<c0516998>] (bus_for_each_drv+0x6c/0xa0)
[<c051692c>] (bus_for_each_drv) from [<c06685ec>] (i2c_del_adapter+0xbc/0x284)
[<c0668530>] (i2c_del_adapter) from [<bf0110ec>] (i2c_imx_remove+0x44/0x164 [i2c_imx])
[<bf0110a8>] (i2c_imx_remove [i2c_imx]) from [<c051a838>] (platform_drv_remove+0x2c/0x44)
[<c051a80c>] (platform_drv_remove) from [<c05183d8>] (__device_release_driver+0x90/0x12c)
[<c0518348>] (__device_release_driver) from [<c051849c>] (device_release_driver+0x28/0x34)
[<c0518474>] (device_release_driver) from [<c0517150>] (unbind_store+0x80/0x104)
[<c05170d0>] (unbind_store) from [<c0516520>] (drv_attr_store+0x28/0x34)
[<c05164f8>] (drv_attr_store) from [<c0298acc>] (sysfs_kf_write+0x50/0x54)
[<c0298a7c>] (sysfs_kf_write) from [<c029801c>] (kernfs_fop_write+0x100/0x214)
[<c0297f1c>] (kernfs_fop_write) from [<c0220130>] (__vfs_write+0x34/0x120)
[<c02200fc>] (__vfs_write) from [<c0221088>] (vfs_write+0xa8/0x170)
[<c0220fe0>] (vfs_write) from [<c0221e74>] (SyS_write+0x4c/0xa8)
[<c0221e28>] (SyS_write) from [<c0108a20>] (ret_fast_syscall+0x0/0x1c)
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
drivers/i2c/i2c-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 1704fc84d647..b432b64e307a 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2179,6 +2179,7 @@ int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
/* add the driver to the list of i2c drivers in the driver core */
driver->driver.owner = owner;
driver->driver.bus = &i2c_bus_type;
+ INIT_LIST_HEAD(&driver->clients);
/* When registration returns, the driver core
* will have called probe() for all matching-but-unbound devices.
@@ -2189,7 +2190,6 @@ int i2c_register_driver(struct module *owner, struct i2c_driver *driver)
pr_debug("driver [%s] registered\n", driver->driver.name);
- INIT_LIST_HEAD(&driver->clients);
/* Walk the adapters that are already present */
i2c_for_each_dev(driver, __process_new_driver);
--
2.8.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] i2c: core: fix NULL pointer dereference under race condition
2016-10-31 19:46 [PATCH] i2c: core: fix NULL pointer dereference under race condition Vladimir Zapolskiy
@ 2016-11-04 19:42 ` Wolfram Sang
0 siblings, 0 replies; 2+ messages in thread
From: Wolfram Sang @ 2016-11-04 19:42 UTC (permalink / raw)
To: Vladimir Zapolskiy; +Cc: Peter Rosin, linux-i2c
[-- Attachment #1: Type: text/plain, Size: 3719 bytes --]
On Mon, Oct 31, 2016 at 09:46:24PM +0200, Vladimir Zapolskiy wrote:
> Race condition between registering an I2C device driver and
> deregistering an I2C adapter device which is assumed to manage that
> I2C device may lead to a NULL pointer dereference due to the
> uninitialized list head of driver clients.
>
> The root cause of the issue is that the I2C bus may know about the
> registered device driver and thus it is matched by bus_for_each_drv(),
> but the list of clients is not initialized and commonly it is NULL,
> because I2C device drivers define struct i2c_driver as static and
> clients field is expected to be initialized by I2C core:
>
> i2c_register_driver() i2c_del_adapter()
> driver_register() ...
> bus_add_driver() ...
> ... bus_for_each_drv(..., __process_removed_adapter)
> ... i2c_do_del_adapter()
> ... list_for_each_entry_safe(..., &driver->clients, ...)
> INIT_LIST_HEAD(&driver->clients);
>
> To solve the problem it is sufficient to do clients list head
> initialization before calling driver_register().
>
> The problem was found while using an I2C device driver with a sluggish
That one must have been really sluggish :)
> registration routine on a bus provided by a physically detachable I2C
> master controller, but practically the oops may be reproduced under
> the race between arbitraty I2C device driver registration and managing
> I2C bus device removal e.g. by unbinding the latter over sysfs:
>
> % echo 21a4000.i2c > /sys/bus/platform/drivers/imx-i2c/unbind
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> Internal error: Oops: 17 [#1] SMP ARM
> CPU: 2 PID: 533 Comm: sh Not tainted 4.9.0-rc3+ #61
> Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
> task: e5ada400 task.stack: e4936000
> PC is at i2c_do_del_adapter+0x20/0xcc
> LR is at __process_removed_adapter+0x14/0x1c
> Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
> Control: 10c5387d Table: 35bd004a DAC: 00000051
> Process sh (pid: 533, stack limit = 0xe4936210)
> Stack: (0xe4937d28 to 0xe4938000)
> Backtrace:
> [<c0667be0>] (i2c_do_del_adapter) from [<c0667cc0>] (__process_removed_adapter+0x14/0x1c)
> [<c0667cac>] (__process_removed_adapter) from [<c0516998>] (bus_for_each_drv+0x6c/0xa0)
> [<c051692c>] (bus_for_each_drv) from [<c06685ec>] (i2c_del_adapter+0xbc/0x284)
> [<c0668530>] (i2c_del_adapter) from [<bf0110ec>] (i2c_imx_remove+0x44/0x164 [i2c_imx])
> [<bf0110a8>] (i2c_imx_remove [i2c_imx]) from [<c051a838>] (platform_drv_remove+0x2c/0x44)
> [<c051a80c>] (platform_drv_remove) from [<c05183d8>] (__device_release_driver+0x90/0x12c)
> [<c0518348>] (__device_release_driver) from [<c051849c>] (device_release_driver+0x28/0x34)
> [<c0518474>] (device_release_driver) from [<c0517150>] (unbind_store+0x80/0x104)
> [<c05170d0>] (unbind_store) from [<c0516520>] (drv_attr_store+0x28/0x34)
> [<c05164f8>] (drv_attr_store) from [<c0298acc>] (sysfs_kf_write+0x50/0x54)
> [<c0298a7c>] (sysfs_kf_write) from [<c029801c>] (kernfs_fop_write+0x100/0x214)
> [<c0297f1c>] (kernfs_fop_write) from [<c0220130>] (__vfs_write+0x34/0x120)
> [<c02200fc>] (__vfs_write) from [<c0221088>] (vfs_write+0xa8/0x170)
> [<c0220fe0>] (vfs_write) from [<c0221e74>] (SyS_write+0x4c/0xa8)
> [<c0221e28>] (SyS_write) from [<c0108a20>] (ret_fast_syscall+0x0/0x1c)
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Good catch! Applied to for-current, thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2016-11-04 19:42 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-31 19:46 [PATCH] i2c: core: fix NULL pointer dereference under race condition Vladimir Zapolskiy
2016-11-04 19:42 ` Wolfram Sang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).