| 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
| ||
|
Date: Mon, 9 Mar 2015 18:09:24 -0500
From: David Fries <david@...es.net>
To: Thorsten Bschorr <thorsten@...horr.de>
Cc: ??????? ??????? <zbr@...emap.net>,
Jonathan ALIBERT <jonathan.alibert@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm
On Mon, Mar 09, 2015 at 11:47:24PM +0100, Thorsten Bschorr wrote:
> > Here's a different strategy, add a w1_therm family_data specific mutex
> > so w1_therm_remove_slave won't return while another thread is in
> > w1_slave_show. Thoughts?
> >
> > I included three patches, the first is the proposed fix, the second
> > makes it more reproducible (and since my testing doesn't have external
> > power I had to ignore that check), the third is just some debugging
> > messages. For testing I'm starting a read from w1_slave then manually
> > remove the sensor, which seems to do the trick.
>
> I'll test your patch, but keeping the original sleep-time tm.
Oops, sorry, it got lost in the shuffle, here's the first patch again
(the others were for debugging and increase that time and so wouldn't
go upstream anyway).
>From 777f5fd75f5f99a3352863e83d226c7b65ebdaa4 Mon Sep 17 00:00:00 2001
From: David Fries <David@...es.net>
Date: Sat, 7 Mar 2015 22:25:37 -0600
Subject: [PATCH] w1_therm, don't let the slave go away while in w1_slave_show
A temperature conversion can take 750 ms to complete, if the sensor is
externally powered it releases the bus_mutex while it waits, but if
the slave device is removed sl becomes a dangling pointer.
The race condition window can be 10 * 750 ms = 7.5 seconds if the crc
check fails.
Reported-By: Thorsten Bschorr <thorsten@...horr.de>
Signed-off-by: David Fries <David@...es.net>
---
drivers/w1/slaves/w1_therm.c | 51 +++++++++++++++++++++++++++++++++-----------
1 file changed, 38 insertions(+), 13 deletions(-)
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index 1f11a20..39a9e6a 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -59,9 +59,20 @@ MODULE_ALIAS("w1-family-" __stringify(W1_THERM_DS28EA00));
static int w1_strong_pullup = 1;
module_param_named(strong_pullup, w1_strong_pullup, int, 0);
+struct w1_therm_family_data {
+ uint8_t rom[9];
+ struct mutex lock;
+};
+
+/* return the address of the lock in the family data */
+#define THERM_LOCK(sl) \
+ (&((struct w1_therm_family_data*)sl->family_data)->lock)
+
static int w1_therm_add_slave(struct w1_slave *sl)
{
- sl->family_data = kzalloc(9, GFP_KERNEL);
+ sl->family_data = kzalloc(sizeof(struct w1_therm_family_data),
+ GFP_KERNEL);
+ mutex_init(THERM_LOCK(sl));
if (!sl->family_data)
return -ENOMEM;
return 0;
@@ -69,6 +80,11 @@ static int w1_therm_add_slave(struct w1_slave *sl)
static void w1_therm_remove_slave(struct w1_slave *sl)
{
+ /* Getting the lock means w1_slave_show isn't sleeping and the
+ * family_data can be freed.
+ */
+ mutex_lock(THERM_LOCK(sl));
+ mutex_unlock(THERM_LOCK(sl));
kfree(sl->family_data);
sl->family_data = NULL;
}
@@ -194,13 +210,15 @@ static ssize_t w1_slave_show(struct device *device,
struct w1_slave *sl = dev_to_w1_slave(device);
struct w1_master *dev = sl->master;
u8 rom[9], crc, verdict, external_power;
- int i, max_trying = 10;
+ int i, ret, max_trying = 10;
ssize_t c = PAGE_SIZE;
- i = mutex_lock_interruptible(&dev->bus_mutex);
- if (i != 0)
- return i;
+ ret = mutex_lock_interruptible(&dev->bus_mutex);
+ if (ret != 0)
+ goto post_unlock;
+ /* prevent the slave from going away in sleep */
+ mutex_lock(THERM_LOCK(sl));
memset(rom, 0, sizeof(rom));
while (max_trying--) {
@@ -230,17 +248,19 @@ static ssize_t w1_slave_show(struct device *device,
mutex_unlock(&dev->bus_mutex);
sleep_rem = msleep_interruptible(tm);
- if (sleep_rem != 0)
- return -EINTR;
+ if (sleep_rem != 0) {
+ ret = -EINTR;
+ goto post_unlock;
+ }
- i = mutex_lock_interruptible(&dev->bus_mutex);
- if (i != 0)
- return i;
+ ret = mutex_lock_interruptible(&dev->bus_mutex);
+ if (ret != 0)
+ goto post_unlock;
} else if (!w1_strong_pullup) {
sleep_rem = msleep_interruptible(tm);
if (sleep_rem != 0) {
- mutex_unlock(&dev->bus_mutex);
- return -EINTR;
+ ret = -EINTR;
+ goto pre_unlock;
}
}
@@ -279,9 +299,14 @@ static ssize_t w1_slave_show(struct device *device,
c -= snprintf(buf + PAGE_SIZE - c, c, "t=%d\n",
w1_convert_temp(rom, sl->family->fid));
+ ret = PAGE_SIZE - c;
+
+pre_unlock:
mutex_unlock(&dev->bus_mutex);
- return PAGE_SIZE - c;
+post_unlock:
+ mutex_unlock(THERM_LOCK(sl));
+ return ret;
}
static int __init w1_therm_init(void)
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists