[PATCH v3] MAX1111: Fix Race condition causing NULL pointer exception

[PATCH v3] MAX1111: Fix Race condition causing NULL pointer exception

[Pavel Herrmann]

spi_sync call uses its spi_message parameter to keep completion information,
using a drvdata structure is not thread-safe, potentially causing one thread
having pointers to memory on or above other threads stack. use mutex to
prevent multiple access

Signed-off-by: Pavel Herrmann <morpheus.ibis@gmail.com>
Acked-by: Russell King <rmk+kernel@arm.linux.org.uk>
Acked-by: Pavel Machek <pavel@ucw.cz>
Acked-by: Marek Vasut <marek.vasut@gmail.com>
Acked-by: Cyril Hrubis <metan@ucw.cz>
Tested-by: Stanislav Brabec <utx@penguin.cz>
---
 drivers/hwmon/max1111.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/hwmon/max1111.c b/drivers/hwmon/max1111.c
index 12a54aa..5c9e54c 100644
--- a/drivers/hwmon/max1111.c
+++ b/drivers/hwmon/max1111.c
@@ -40,6 +40,8 @@ struct max1111_data {
 	struct spi_transfer	xfer[2];
 	uint8_t *tx_buf;
 	uint8_t *rx_buf;
+	struct mutex		drvdata_lock;
+	/* protect msg, xfer and buffers from multiple access */
 };
 
 static int max1111_read(struct device *dev, int channel)
@@ -48,6 +50,9 @@ static int max1111_read(struct device *dev, int channel)
 	uint8_t v1, v2;
 	int err;
 
+	/* writing to drvdata struct is not thread safe, wait on mutex */
+	mutex_lock(&data->drvdata_lock);
+
 	data->tx_buf[0] = (channel << MAX1111_CTRL_SEL_SH) |
 		MAX1111_CTRL_PD0 | MAX1111_CTRL_PD1 |
 		MAX1111_CTRL_SGL | MAX1111_CTRL_UNI | MAX1111_CTRL_STR;
@@ -55,12 +60,15 @@ static int max1111_read(struct device *dev, int channel)
 	err = spi_sync(data->spi, &data->msg);
 	if (err < 0) {
 		dev_err(dev, "spi_sync failed with %d\n", err);
+		mutex_unlock(&data->drvdata_lock);
 		return err;
 	}
 
 	v1 = data->rx_buf[0];
 	v2 = data->rx_buf[1];
 
+	mutex_unlock(&data->drvdata_lock);
+
 	if ((v1 & 0xc0) || (v2 & 0x3f))
 		return -EINVAL;
 
@@ -176,6 +184,8 @@ static int __devinit max1111_probe(struct spi_device *spi)
 	if (err)
 		goto err_free_data;
 
+	mutex_init(&data->drvdata_lock);
+
 	data->spi = spi;
 	spi_set_drvdata(spi, data);
 
@@ -213,6 +223,7 @@ static int __devexit max1111_remove(struct spi_device *spi)
 
 	hwmon_device_unregister(data->hwmon_dev);
 	sysfs_remove_group(&spi->dev.kobj, &max1111_attr_group);
+	mutex_destroy(data->drvdata_lock);
 	kfree(data->rx_buf);
 	kfree(data->tx_buf);
 	kfree(data);
-- 
1.7.5.rc3

[PATCH v3] MAX1111: Fix Race condition causing NULL pointer exception

[Jean Delvare]

On Mon, 11 Jul 2011 23:50:38 +0200, Pavel Herrmann wrote:
> spi_sync call uses its spi_message parameter to keep completion information,
> using a drvdata structure is not thread-safe, potentially causing one thread
> having pointers to memory on or above other threads stack. use mutex to
> prevent multiple access

Honestly, I have no idea what "causing one thread having pointers to
memory on or above other threads stack" means (nor why this would be
bad.)

Patch applied nevertheless, as it fixes an actual bug which should be
fixed ASAP. Thanks for your contribution.

-- 
Jean Delvare

[PATCH v3] MAX1111: Fix Race condition causing NULL pointer exception

[Pavel Herrmann]

On Tuesday 12 of July 2011 09:36:06 Jean Delvare wrote:
> On Mon, 11 Jul 2011 23:50:38 +0200, Pavel Herrmann wrote:
> > spi_sync call uses its spi_message parameter to keep completion
> > information, using a drvdata structure is not thread-safe, potentially
> > causing one thread having pointers to memory on or above other threads
> > stack. use mutex to prevent multiple access
> 
> Honestly, I have no idea what "causing one thread having pointers to
> memory on or above other threads stack" means (nor why this would be
> bad.)

the long-winded story is that thread A writes a pointer onto its stack into 
the drvdata as part of spi_sync call, then thread B comes in and puts a 
pointer onto its stack into the drvdata, at the end of spi_sync thread A uses 
this pointer (assuming it is unchanged), which is pointing either onto valid 
stack of thread B or somewhere above it (if thread B already returned)

> Patch applied nevertheless, as it fixes an actual bug which should be
> fixed ASAP. Thanks for your contribution.

thanks

[lm-sensors] [PATCH v3] MAX1111: Fix Race condition causing NULL pointer exception

[Mark Brown]

On Tue, Jul 12, 2011 at 10:04:55AM +0200, Pavel Herrmann wrote:
> On Tuesday 12 of July 2011 09:36:06 Jean Delvare wrote:

> > Honestly, I have no idea what "causing one thread having pointers to
> > memory on or above other threads stack" means (nor why this would be
> > bad.)

> the long-winded story is that thread A writes a pointer onto its stack into 
> the drvdata as part of spi_sync call, then thread B comes in and puts a 
> pointer onto its stack into the drvdata, at the end of spi_sync thread A uses 
> this pointer (assuming it is unchanged), which is pointing either onto valid 
> stack of thread B or somewhere above it (if thread B already returned)

That's just a use after free bug, the fact that the variables are on the
stacks of other threads isn't the issue, the issue is that the two
threads that are sharing state arne't properly synchronized.

[PATCH v3] MAX1111: Fix Race condition causing NULL pointer exception

[Jean Delvare]

On Tue, 12 Jul 2011 10:04:55 +0200, Pavel Herrmann wrote:
> On Tuesday 12 of July 2011 09:36:06 Jean Delvare wrote:
> > On Mon, 11 Jul 2011 23:50:38 +0200, Pavel Herrmann wrote:
> > > spi_sync call uses its spi_message parameter to keep completion
> > > information, using a drvdata structure is not thread-safe, potentially
> > > causing one thread having pointers to memory on or above other threads
> > > stack. use mutex to prevent multiple access
> > 
> > Honestly, I have no idea what "causing one thread having pointers to
> > memory on or above other threads stack" means (nor why this would be
> > bad.)
> 
> the long-winded story is that thread A writes a pointer onto its stack into 
> the drvdata as part of spi_sync call, then thread B comes in and puts a 
> pointer onto its stack into the drvdata, at the end of spi_sync thread A uses 
> this pointer (assuming it is unchanged), which is pointing either onto valid 
> stack of thread B or somewhere above it (if thread B already returned)

OK, I understand now, thanks for the explanation. But these low-level
technical details have little interest for explaining what your patch
does and why it is needed.

Here, the key problem was that the code assumed exclusive access to
drvdata for the duration of max1111_read() but no mechanism was in
place to guarantee this. It's not even limited to spi_sync and how it
works internally. Just looking at max1111_read() without any knowledge
of the spi subsystem, it is pretty clear that data->tx_buf and
data->rx_buf could get corrupted if max1111_read() runs twice in
parallel. As a matter of fact, your fix doesn't just protect the call
to spi_sync(), it protects data->tx_buf and data->rx_buf too.

The stack thing may be how the bug manifested in your case, but once
the issue is understood as a whole, the patch description should not
include more details than needed. You should really limit yourself to
the higher level needed to understand why the change is necessary and
correct.

Thanks,
-- 
Jean Delvare