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: Sun, 1 Mar 2015 18:17:13 -0600 From: David Fries <david@...es.net> To: Thorsten Bschorr <thorsten@...horr.de> Cc: Evgeniy Polyakov <zbr@...emap.net>, linux-kernel@...r.kernel.org Subject: Re: [PATCH] Avoid null-pointer access in w1/slaves/w1_therm On Sun, Mar 01, 2015 at 02:04:53PM +0100, Thorsten Bschorr wrote: > Hi David, > > thanks for your feedback on my first patch, I wasn't aware of checkpatch.pl. > > Initially, I had just if-ed the usage of family-data, which did not > look that nice. I was referring to this proof-of-concept workaround in > my initial bug report. > > The patch I've submitted is different from my proof-of-concept > workaround. Not unlocking the bus before returning clearly is an > error, I did not extensively test this patch. > > > > or just increment it while sleeping, which is when it's needed, which > > also looks simpler. > > > > if (external_power) { > > + int refcnt; > > mutex_unlock(&dev->bus_mutex); > > > > + /* prevent the slave from going away */ > > + atomic_inc(&sl->refcnt); > > sleep_rem = msleep_interruptible(tm); > > + refcnt = w1_unref_slave(sl); > > - if (sleep_rem != 0) > > + if (sleep_rem != 0 || !refcnt) > > return -EINTR; > > > > i = mutex_lock_interruptible(&dev->bus_mutex); > > if (i != 0) > > return i; > > } else if (!w1_strong_pullup) { > > > I like this better than my workaround-patch. > > One thought occurred to me when looking at this proposal: wouldn't it > be even better to increase sl->refcnt before unlocking the mutex? > I was asking myself if it is possible that the current thread gets > suspended between mutex_unlock(&dev->bus_mutex); and > atomic_inc(&sl->refcnt); thus leaving another thread the change to > unref the device? > (I'm not that familiar with linux scheduling, so my assumption might be void.) You are correct, it would be a race condition if it doesn't increment the refcnt before unlocking the mutex, and it should get the mutex before unref. Here's an updated version, I haven't even tried to compile it. What do you think Evgeniy? if (external_power) { int refcnt; /* prevent the slave from going away in sleep */ atomic_inc(&sl->refcnt); mutex_unlock(&dev->bus_mutex); sleep_rem = msleep_interruptible(tm); if (sleep_rem != 0) { w1_unref_slave(sl); return -EINTR; } i = mutex_lock_interruptible(&dev->bus_mutex); refcnt = w1_unref_slave(sl); if (i != 0) { /* failed to lock */ return i; } if (!refcnt) /* got lock, but slave went away */ mutex_unlock(&dev->bus_mutex); return -EINTR; } } else if (!w1_strong_pullup) { -- David Fries <david@...es.net> PGP pub CB1EE8F0 http://fries.net/~david/ -- 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