[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Hugo Lefeuvre]

Add a rw semaphore fixing potential NULL pointer dereferences in the
pi433 driver.

If pi433_release and pi433_ioctl are concurrently called,
pi433_release might set filp->private_data to NULL while pi433_ioctl
is still accessing it, leading to NULL pointer dereference. This issue
might also affect pi433_write and pi433_read.

The newly introduced semaphore makes sure that filp->private_data
will not be freed by pi433_release (writer) as long as pi433_write,
pi433_read or pi433_ioctl (readers) are still executing.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 drivers/staging/pi433/pi433_if.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d1e0ddbc79ce..ce83139cc795 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -39,6 +39,7 @@
 #include <linux/kfifo.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
+#include <linux/rwsem.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/interrupt.h>
@@ -116,6 +117,7 @@ struct pi433_device {
 struct pi433_instance {
 	struct pi433_device	*device;
 	struct pi433_tx_cfg	tx_cfg;
+	struct rw_semaphore     instance_sem;
 };
 
 /*-------------------------------------------------------------------------*/
@@ -778,6 +780,7 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	if (size > MAX_MSG_SIZE)
 		return -EMSGSIZE;
 
+	down_read(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 
@@ -785,6 +788,7 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	mutex_lock(&device->rx_lock);
 	if (device->rx_active) {
 		mutex_unlock(&device->rx_lock);
+		up_read(&instance->instance_sem);
 		return -EAGAIN;
 	}
 
@@ -805,9 +809,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	if (bytes_received > 0) {
 		retval = copy_to_user(buf, device->rx_buffer, bytes_received);
 		if (retval)
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 	}
 
+	up_read(&instance->instance_sem);
 	return bytes_received;
 }
 
@@ -820,11 +826,13 @@ pi433_write(struct file *filp, const char __user *buf,
 	int                     retval;
 	unsigned int		copied;
 
+	down_read(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 
 	/* check, whether internal buffer (tx thread) is big enough for requested size */
 	if (count > MAX_MSG_SIZE)
+		up_read(&instance->instance_sem);
 		return -EMSGSIZE;
 
 	/* write the following sequence into fifo:
@@ -851,6 +859,7 @@ pi433_write(struct file *filp, const char __user *buf,
 	/* start transfer */
 	wake_up_interruptible(&device->tx_wait_queue);
 	dev_dbg(device->dev, "write: generated new msg with %d bytes.", copied);
+	up_read(&instance->instance_sem);
 
 	return copied;
 
@@ -858,6 +867,7 @@ pi433_write(struct file *filp, const char __user *buf,
 	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
 	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
 	mutex_unlock(&device->tx_fifo_lock);
+	up_read(&instance->instance_sem);
 	return -EAGAIN;
 }
 
@@ -873,29 +883,31 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (_IOC_TYPE(cmd) != PI433_IOC_MAGIC)
 		return -ENOTTY;
 
-	/* TODO? guard against device removal before, or while,
-	 * we issue this ioctl. --> device_get()
-	 */
+	down_read(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 
 	if (!device)
+		up_read(&instance->instance_sem);
 		return -ESHUTDOWN;
 
 	switch (cmd) {
 	case PI433_IOC_RD_TX_CFG:
 		if (copy_to_user(argp, &instance->tx_cfg,
 				 sizeof(struct pi433_tx_cfg)))
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		break;
 	case PI433_IOC_WR_TX_CFG:
 		if (copy_from_user(&instance->tx_cfg, argp,
 				   sizeof(struct pi433_tx_cfg)))
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		break;
 	case PI433_IOC_RD_RX_CFG:
 		if (copy_to_user(argp, &device->rx_cfg,
 				 sizeof(struct pi433_rx_cfg)))
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		break;
 	case PI433_IOC_WR_RX_CFG:
@@ -904,21 +916,26 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		/* during pendig read request, change of config not allowed */
 		if (device->rx_active) {
 			mutex_unlock(&device->rx_lock);
+			up_read(&instance->instance_sem);
 			return -EAGAIN;
 		}
 
 		if (copy_from_user(&device->rx_cfg, argp,
 				   sizeof(struct pi433_rx_cfg))) {
 			mutex_unlock(&device->rx_lock);
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		}
 
 		mutex_unlock(&device->rx_lock);
+		up_read(&instance->instance_sem);
 		break;
 	default:
+		up_read(&instance->instance_sem);
 		retval = -EINVAL;
 	}
 
+	up_read(&instance->instance_sem);
 	return retval;
 }
 
@@ -964,6 +981,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
 	/* setup instance data*/
 	instance->device = device;
 	instance->tx_cfg.bit_rate = 4711;
+	init_rwsem(&instance->instance_sem);
 	// TODO: fill instance->tx_cfg;
 
 	/* instance data as context */
@@ -978,6 +996,7 @@ static int pi433_release(struct inode *inode, struct file *filp)
 	struct pi433_instance	*instance;
 	struct pi433_device	*device;
 
+	down_write(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 	kfree(instance);
-- 
2.17.1

[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Hugo Lefeuvre]

Add a rw semaphore fixing potential NULL pointer dereferences in the
pi433 driver.

If pi433_release and pi433_ioctl are concurrently called,
pi433_release might set filp->private_data to NULL while pi433_ioctl
is still accessing it, leading to NULL pointer dereference. This issue
might also affect pi433_write and pi433_read.

The newly introduced semaphore makes sure that filp->private_data
will not be freed by pi433_release (writer) as long as pi433_write,
pi433_read or pi433_ioctl (readers) are still executing.

Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
---
 drivers/staging/pi433/pi433_if.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d1e0ddbc79ce..ce83139cc795 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -39,6 +39,7 @@
 #include <linux/kfifo.h>
 #include <linux/errno.h>
 #include <linux/mutex.h>
+#include <linux/rwsem.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/interrupt.h>
@@ -116,6 +117,7 @@ struct pi433_device {
 struct pi433_instance {
 	struct pi433_device	*device;
 	struct pi433_tx_cfg	tx_cfg;
+	struct rw_semaphore     instance_sem;
 };
 
 /*-------------------------------------------------------------------------*/
@@ -778,6 +780,7 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	if (size > MAX_MSG_SIZE)
 		return -EMSGSIZE;
 
+	down_read(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 
@@ -785,6 +788,7 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	mutex_lock(&device->rx_lock);
 	if (device->rx_active) {
 		mutex_unlock(&device->rx_lock);
+		up_read(&instance->instance_sem);
 		return -EAGAIN;
 	}
 
@@ -805,9 +809,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
 	if (bytes_received > 0) {
 		retval = copy_to_user(buf, device->rx_buffer, bytes_received);
 		if (retval)
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 	}
 
+	up_read(&instance->instance_sem);
 	return bytes_received;
 }
 
@@ -820,11 +826,13 @@ pi433_write(struct file *filp, const char __user *buf,
 	int                     retval;
 	unsigned int		copied;
 
+	down_read(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 
 	/* check, whether internal buffer (tx thread) is big enough for requested size */
 	if (count > MAX_MSG_SIZE)
+		up_read(&instance->instance_sem);
 		return -EMSGSIZE;
 
 	/* write the following sequence into fifo:
@@ -851,6 +859,7 @@ pi433_write(struct file *filp, const char __user *buf,
 	/* start transfer */
 	wake_up_interruptible(&device->tx_wait_queue);
 	dev_dbg(device->dev, "write: generated new msg with %d bytes.", copied);
+	up_read(&instance->instance_sem);
 
 	return copied;
 
@@ -858,6 +867,7 @@ pi433_write(struct file *filp, const char __user *buf,
 	dev_dbg(device->dev, "write to fifo failed: 0x%x", retval);
 	kfifo_reset(&device->tx_fifo); // TODO: maybe find a solution, not to discard already stored, valid entries
 	mutex_unlock(&device->tx_fifo_lock);
+	up_read(&instance->instance_sem);
 	return -EAGAIN;
 }
 
@@ -873,29 +883,31 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 	if (_IOC_TYPE(cmd) != PI433_IOC_MAGIC)
 		return -ENOTTY;
 
-	/* TODO? guard against device removal before, or while,
-	 * we issue this ioctl. --> device_get()
-	 */
+	down_read(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 
 	if (!device)
+		up_read(&instance->instance_sem);
 		return -ESHUTDOWN;
 
 	switch (cmd) {
 	case PI433_IOC_RD_TX_CFG:
 		if (copy_to_user(argp, &instance->tx_cfg,
 				 sizeof(struct pi433_tx_cfg)))
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		break;
 	case PI433_IOC_WR_TX_CFG:
 		if (copy_from_user(&instance->tx_cfg, argp,
 				   sizeof(struct pi433_tx_cfg)))
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		break;
 	case PI433_IOC_RD_RX_CFG:
 		if (copy_to_user(argp, &device->rx_cfg,
 				 sizeof(struct pi433_rx_cfg)))
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		break;
 	case PI433_IOC_WR_RX_CFG:
@@ -904,21 +916,26 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 		/* during pendig read request, change of config not allowed */
 		if (device->rx_active) {
 			mutex_unlock(&device->rx_lock);
+			up_read(&instance->instance_sem);
 			return -EAGAIN;
 		}
 
 		if (copy_from_user(&device->rx_cfg, argp,
 				   sizeof(struct pi433_rx_cfg))) {
 			mutex_unlock(&device->rx_lock);
+			up_read(&instance->instance_sem);
 			return -EFAULT;
 		}
 
 		mutex_unlock(&device->rx_lock);
+		up_read(&instance->instance_sem);
 		break;
 	default:
+		up_read(&instance->instance_sem);
 		retval = -EINVAL;
 	}
 
+	up_read(&instance->instance_sem);
 	return retval;
 }
 
@@ -964,6 +981,7 @@ static int pi433_open(struct inode *inode, struct file *filp)
 	/* setup instance data*/
 	instance->device = device;
 	instance->tx_cfg.bit_rate = 4711;
+	init_rwsem(&instance->instance_sem);
 	// TODO: fill instance->tx_cfg;
 
 	/* instance data as context */
@@ -978,6 +996,7 @@ static int pi433_release(struct inode *inode, struct file *filp)
 	struct pi433_instance	*instance;
 	struct pi433_device	*device;
 
+	down_write(&instance->instance_sem);
 	instance = filp->private_data;
 	device = instance->device;
 	kfree(instance);
-- 
2.17.1

Re: [PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[valdis.kletnieks]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

On Fri, 01 Jun 2018 08:50:37 -0400, Hugo Lefeuvre said:

> @@ -805,9 +809,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
>  	if (bytes_received > 0) {
>  		retval = copy_to_user(buf, device->rx_buffer, bytes_received);
>  		if (retval)
> +			up_read(&instance->instance_sem);
>  			return -EFAULT;
>  	}
>
> +	up_read(&instance->instance_sem);
>  	return bytes_received;
>  }

This doesn't do what you think.

[-- Attachment #2: Type: application/pgp-signature, Size: 486 bytes --]

[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[valdis.kletnieks at vt.edu]

On Fri, 01 Jun 2018 08:50:37 -0400, Hugo Lefeuvre said:

> @@ -805,9 +809,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
>  	if (bytes_received > 0) {
>  		retval = copy_to_user(buf, device->rx_buffer, bytes_received);
>  		if (retval)
> +			up_read(&instance->instance_sem);
>  			return -EFAULT;
>  	}
>
> +	up_read(&instance->instance_sem);
>  	return bytes_received;
>  }

This doesn't do what you think.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180601/136744fb/attachment.sig>

Re: [PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Hugo Lefeuvre]

Hi Valdis,

> > @@ -805,9 +809,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
> >  	if (bytes_received > 0) {
> >  		retval = copy_to_user(buf, device->rx_buffer, bytes_received);
> >  		if (retval)
> > +			up_read(&instance->instance_sem);
> >  			return -EFAULT;
> >  	}
> >
> > +	up_read(&instance->instance_sem);
> >  	return bytes_received;
> >  }
> 
> This doesn't do what you think.

Oh right, no curly braces, didn't notice it. Thanks !

Otherwise, do you think the usage of rw semaphore is appropriate in this
case ?

Regards,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Hugo Lefeuvre]

Hi Valdis,

> > @@ -805,9 +809,11 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos)
> >  	if (bytes_received > 0) {
> >  		retval = copy_to_user(buf, device->rx_buffer, bytes_received);
> >  		if (retval)
> > +			up_read(&instance->instance_sem);
> >  			return -EFAULT;
> >  	}
> >
> > +	up_read(&instance->instance_sem);
> >  	return bytes_received;
> >  }
> 
> This doesn't do what you think.

Oh right, no curly braces, didn't notice it. Thanks !

Otherwise, do you think the usage of rw semaphore is appropriate in this
case ?

Regards,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

Re: [PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Greg Kroah-Hartman]

On Fri, Jun 01, 2018 at 08:50:37AM -0400, Hugo Lefeuvre wrote:
> Add a rw semaphore fixing potential NULL pointer dereferences in the
> pi433 driver.

Unless you can measure the performance difference, do not use a rw
semaphore, just use a normal mutex please.  Odds are it will be faster
in the end and take up less space.

So please test, or if you can't test, just use a mutex.

thanks,

greg k-h

[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Greg Kroah-Hartman]

On Fri, Jun 01, 2018 at 08:50:37AM -0400, Hugo Lefeuvre wrote:
> Add a rw semaphore fixing potential NULL pointer dereferences in the
> pi433 driver.

Unless you can measure the performance difference, do not use a rw
semaphore, just use a normal mutex please.  Odds are it will be faster
in the end and take up less space.

So please test, or if you can't test, just use a mutex.

thanks,

greg k-h

Re: [PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Hugo Lefeuvre]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

Hi,

> Unless you can measure the performance difference, do not use a rw
> semaphore, just use a normal mutex please.  Odds are it will be faster
> in the end and take up less space.
> 
> So please test, or if you can't test, just use a mutex.

I don't have the device yet, so I won't be able to test. I have opted for the
mutex instead.

I have just sent an updated version fixing another issue in my code: The
mutex can't be included in pi433_instance because we have to lock it
before dereferencing our pi433_instance pointer. The best solution I
could think of was to create a wrapper struct pi433_data which would contain
pointers to the pi433_instance and its mutex. I couldn't find any
similar situation in the kernel, so I'm not sure it's the right way to
go though.

Thanks !

Regards,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH] staging: pi433: add rw semaphore fixing concurrency issues

[Hugo Lefeuvre]

Hi,

> Unless you can measure the performance difference, do not use a rw
> semaphore, just use a normal mutex please.  Odds are it will be faster
> in the end and take up less space.
> 
> So please test, or if you can't test, just use a mutex.

I don't have the device yet, so I won't be able to test. I have opted for the
mutex instead.

I have just sent an updated version fixing another issue in my code: The
mutex can't be included in pi433_instance because we have to lock it
before dereferencing our pi433_instance pointer. The best solution I
could think of was to create a wrapper struct pi433_data which would contain
pointers to the pi433_instance and its mutex. I couldn't find any
similar situation in the kernel, so I'm not sure it's the right way to
go though.

Thanks !

Regards,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180602/db6d9bc4/attachment.sig>