Skip to contentSkip to conversation

MAX1111: Fix Race condition causing NULL pointer exception

PATCH Superseded Pavel Herrmann 3 participants 5 messages Jul 11, 2011 – Jul 12, 2011 Hardware monitoring

lore
Share & tools
lore

Pavel Herrmann

author
#0 lore raw

PATCH v3 1/1 MAX1111: Fix Race condition causing NULL pointer exception

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 <[email protected]>
Acked-by Russell King <[email protected]>
Acked-by Pavel Machek <[email protected]>
Acked-by Marek Vasut <[email protected]>
Acked-by Cyril Hrubis <[email protected]>
Tested-by Stanislav Brabec <[email protected]>
---
 drivers/hwmon/max1111.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)
drivers/hwmon/max1111.c +11 -0
index 12a54aa..5c9e54c 100644
@@ -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);
Signature · 1 line
1.7.5.rc3
#1 lore raw

PATCH v3 1/1 MAX1111: Fix Race condition causing NULL pointer exception

Quoted 4 lines · 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

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.

Signature · 1 line
Jean Delvare
author
#2 lore raw

PATCH v3 1/1 MAX1111: Fix Race condition causing NULL pointer exception

Quoted 9 lines · On Tuesday 12 of July 2011 09:36:06 Jean Delvare wrote:
Quoted 4 lines · 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

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)

Quoted 2 lines

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

thanks

#3 lore raw

PATCH v3 1/1 MAX1111: Fix Race condition causing NULL pointer exception

Quoted 11 lines · Pavel Herrmann
Quoted 3 lines · 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.

#4 lore raw

PATCH v3 1/1 MAX1111: Fix Race condition causing NULL pointer exception

In reply to Pavel Herrmann
Quoted 11 lines · Pavel Herrmann
Quoted 9 lines · On Tuesday 12 of July 2011 09:36:06 Jean Delvare wrote:
Quoted 4 lines · 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

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.)

Quoted 5 lines

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,

Signature · 1 line
Jean Delvare