| 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
| ||
|
Message-Id: <20210623003630.274804-1-mcgrof@kernel.org>
Date: Tue, 22 Jun 2021 17:36:30 -0700
From: Luis Chamberlain <mcgrof@...nel.org>
To: gregkh@...uxfoundation.org, rafael@...nel.org
Cc: jeyu@...nel.org, ngupta@...are.org,
sergey.senozhatsky.work@...il.com, minchan@...nel.org,
mcgrof@...nel.org, axboe@...nel.dk, mbenes@...e.com,
jpoimboe@...hat.com, tglx@...utronix.de, keescook@...omium.org,
jikos@...nel.org, rostedt@...dmis.org, peterz@...radead.org,
linux-block@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: [PATCH v3] drivers/base/core: refcount kobject and bus on device attribute read / store
It's possible today to have a device attribute read or store
race against device removal. When this happens there is a small
chance that the dereference for the private data area of the driver
is NULL.
Let's consider the zram driver as an example. Its possible to run into
a race where a sysfs knob is being used, we get preempted, and a zram
device is removed before we complete use of the sysfs knob. This can happen
for instance on block devices, where for instance the zram block devices
just part of the private data of the block device.
For instance this can happen in the following two situations
as examples to illustrate this better:
CPU 1 CPU 2
destroy_devices
...
compact_store()
zram = dev_to_zram(dev);
idr_for_each(zram_remove_cb
zram_remove
...
kfree(zram)
down_read(&zram->init_lock);
CPU 1 CPU 2
hot_remove_store
compact_store()
zram = dev_to_zram(dev);
zram_remove
kfree(zram)
down_read(&zram->init_lock);
To ensure the private data pointer is valid we could use bdget() / bdput()
in between access, however that would mean doing that in all sysfs
reads/stores on the driver. Instead a generic solution for all drivers
is to ensure the device kobject is still valid and also the bus, if
a bus is present.
This issue does not fix a known crash, however this race was
spotted by Minchan Kim through code inspection upon code review
of another zram patch.
Suggested-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>
---
This v3 is the same as v2, but since the I got an email asking me to
clarify the differences between the first iteration and the second, this
v3 just describes what I did not explain in the v2. Namely:
* I removed the checks from get_device() calls as suggested
* The functions which are now being used outside of bus.c have
forwared declarations stuffed into base.h
drivers/base/base.h | 2 ++
drivers/base/bus.c | 4 ++--
drivers/base/core.c | 36 ++++++++++++++++++++++++++++++++----
3 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/drivers/base/base.h b/drivers/base/base.h
index e5f9b7e656c3..3f95b125b667 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -127,6 +127,8 @@ static inline void auxiliary_bus_init(void) { }
struct kobject *virtual_device_parent(struct device *dev);
+extern struct bus_type *bus_get(struct bus_type *bus);
+extern void bus_put(struct bus_type *bus);
extern int bus_add_device(struct device *dev);
extern void bus_probe_device(struct device *dev);
extern void bus_remove_device(struct device *dev);
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 36d0c654ea61..21c80d7d6433 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -39,7 +39,7 @@ static struct kset *system_kset;
static int __must_check bus_rescan_devices_helper(struct device *dev,
void *data);
-static struct bus_type *bus_get(struct bus_type *bus)
+struct bus_type *bus_get(struct bus_type *bus)
{
if (bus) {
kset_get(&bus->p->subsys);
@@ -48,7 +48,7 @@ static struct bus_type *bus_get(struct bus_type *bus)
return NULL;
}
-static void bus_put(struct bus_type *bus)
+void bus_put(struct bus_type *bus)
{
if (bus)
kset_put(&bus->p->subsys);
diff --git a/drivers/base/core.c b/drivers/base/core.c
index 4a8bf8cda52b..f69aa040b56d 100644
--- a/drivers/base/core.c
+++ b/drivers/base/core.c
@@ -2042,28 +2042,56 @@ EXPORT_SYMBOL(dev_driver_string);
static ssize_t dev_attr_show(struct kobject *kobj, struct attribute *attr,
char *buf)
{
- struct device_attribute *dev_attr = to_dev_attr(attr);
- struct device *dev = kobj_to_dev(kobj);
+ struct device_attribute *dev_attr;
+ struct device *dev;
+ struct bus_type *bus = NULL;
ssize_t ret = -EIO;
+ dev = get_device(kobj_to_dev(kobj));
+ if (dev->bus) {
+ bus = bus_get(dev->bus);
+ if (!bus)
+ goto out;
+ }
+
+ dev_attr = to_dev_attr(attr);
if (dev_attr->show)
ret = dev_attr->show(dev, dev_attr, buf);
if (ret >= (ssize_t)PAGE_SIZE) {
printk("dev_attr_show: %pS returned bad count\n",
dev_attr->show);
}
+
+ bus_put(bus);
+out:
+ put_device(dev);
+
return ret;
}
static ssize_t dev_attr_store(struct kobject *kobj, struct attribute *attr,
const char *buf, size_t count)
{
- struct device_attribute *dev_attr = to_dev_attr(attr);
- struct device *dev = kobj_to_dev(kobj);
+ struct device_attribute *dev_attr;
+ struct device *dev;
+ struct bus_type *bus = NULL;
ssize_t ret = -EIO;
+ dev = get_device(kobj_to_dev(kobj));
+ if (dev->bus) {
+ bus = bus_get(dev->bus);
+ if (!bus)
+ goto out;
+ }
+
+ dev_attr = to_dev_attr(attr);
if (dev_attr->store)
ret = dev_attr->store(dev, dev_attr, buf, count);
+
+ bus_put(bus);
+out:
+ put_device(dev);
+
return ret;
}
--
2.30.2
Powered by blists - more mailing lists