MAX1111: Fix Race condition causing NULL pointer exception

spi_sync call uses its spi_message parameter to keep completion information, having this structure static 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]>

---
 drivers/hwmon/max1111.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

drivers/hwmon/max1111.c +0 -0

index 12a54aa..d872f57 100644

drivers/hwmon/max1111.c +7 -0

+	mutex_lock(&data->msg_lock_mutex);
+
 	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 +61,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->msg_lock_mutex);
 		return err;
 	}
 
 	v1 = data->rx_buf[0];
 	v2 = data->rx_buf[1];
 
+	mutex_unlock(&data->msg_lock_mutex);
+
 	if ((v1 & 0xc0) || (v2 & 0x3f))
 		return -EINVAL;
 

@@ -176,6 +185,8 @@ static int __devinit max1111_probe(struct spi_device *spi)
 	if (err)
 		goto err_free_data;
 
+	mutex_init(&data->msg_lock_mutex);
+
 	data->spi = spi;
 	spi_set_drvdata(spi, data);
 

Quoted 4 lines

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

This has nothing to do with static, as a matter of fact the structure is dynamically allocated. The bottom line is that the driver structure is such that calls to max1111_read() must be serialized.

drivers/hwmon/max1111.c static int max1111_read(struct device *dev, int channel) · line 54

Quoted 3 lines

drivers/hwmon/max1111.c +3 -0

+	/* spi_sync requires data not to be freed before function returns
+	 * for static data, any access is dangerous, use locks
+	 */

This has nothing to do with "freeing data". max1111_read() doesn't free anything. It is making use of a data structure, the access to which must be serialized. Easy as that. And no, access isn't dangerous ;)

Please respin your patch with a better struct member name and improved description and comments, and I'll be happy to apply it.

Quoted 5 lines · On Monday 11 of July 2011 22:11:48 Jean Delvare wrote:

Quoted 4 lines

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

Quoted 3 lines

This has nothing to do with static, as a matter of fact the structure is dynamically allocated. The bottom line is that the driver structure is such that calls to max1111_read() must be serialized.

the structure is dynamically allocated, but the pointer used to hold it is a static global var.

"static" in this context meant "shared by all threads"

Quoted 2 lines · Pavel Herrmann

the structure is dynamically allocated, but the pointer used to hold it is a static global var.

This is true only if CONFIG_SHARPSL_PM is defined, and it assumes that the driver is instantiated exactly once. That is pretty badly broken (the commit introducing it even admits that), and should be fixed. This does not happen CONFIG_SHARPSL_PM is not defined. If CONFIG_SHARPSL_PM _is_ defined in your environment, and you do have multiple instances of the driver (ie if you have multiple MAX1111 chips in your system), a severe problem is that max1111_read_channel() does not identify the driver instance. That can not be fixed with a mutex.

I think it would make sense to stick with common terminology. In your definition, almost all global variables of all programs out there would be defined as "static".

Quoted 4 lines · On Monday 11 of July 2011 14:03:13 Guenter Roeck wrote:

Quoted 2 lines · Pavel Herrmann

the structure is dynamically allocated, but the pointer used to hold it is a static global var.

Quoted 8 lines

This is true only if CONFIG_SHARPSL_PM is defined, and it assumes that the driver is instantiated exactly once. That is pretty badly broken (the commit introducing it even admits that), and should be fixed. This does not happen CONFIG_SHARPSL_PM is not defined. If CONFIG_SHARPSL_PM _is_ defined in your environment, and you do have multiple instances of the driver (ie if you have multiple MAX1111 chips in your system), a severe problem is that max1111_read_channel() does not identify the driver instance. That can not be fixed with a mutex.

if you don't have CONFIG_SHARPSL_PM then there is nothing calling max1111_read, and thus any of the discussed doesn't matter

AFAIK max1111 is only used in sharpsl devices (according to kernel drivers anyways), and only one a piece.
this patch is meant to fix a crash, not make the driver code pretty just in case someone else decides to use it. this patch also doesn't present any more challenges for solving the multiple devices issue and would be necessary either way, as drvdata is not thread-safe anyways (or I am badly mistaken)

spi_sync call uses its spi_message parameter to keep completion information, having this structure static 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]>