| Message ID | 1310421038-14823-1-git-send-email-morpheus.ibis@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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,
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);