Re: [PATCH] staging: pi433: add mutex fixing race condition when accessing tx_cfg

[Hugo Lefeuvre]

> We read the data from the user here and then we write it to the fifo
> in pi433_write().  We should be using the device->tx_fifo_lock so that
> we don't copy over the data at the same time we're writing it to the
> fifo.

Oh right, that makes the bug even worse.

In this case we don't even need to introduce a new lock, using
device->tx_fifo_lock should be fine. I'll update the patch.

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


signature.asc
Description: PGP signature
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] staging: pi433: add mutex fixing race condition when accessing tx_cfg

[Hugo Lefeuvre]

In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is
modified using

copy_from_user(>tx_cfg, argp, sizeof(struct pi433_tx_cfg)))

without any kind of synchronization. In the case where two threads
would execute this same command concurrently the tx_cfg field might
enter in an inconsistent state.

Add a mutex making sure that the PI433_IOC_WR_TX_CFG case will never
be run by several threads concurrently.

Signed-off-by: Hugo Lefeuvre 
---
 drivers/staging/pi433/pi433_if.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index d1e0ddbc79ce..94c9d5482f44 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -115,6 +115,7 @@ struct pi433_device {
 
 struct pi433_instance {
struct pi433_device *device;
+   struct mutextx_cfg_lock; /* guards race conditions when 
updating tx config */
struct pi433_tx_cfg tx_cfg;
 };
 
@@ -889,9 +890,13 @@ pi433_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
return -EFAULT;
break;
case PI433_IOC_WR_TX_CFG:
+   mutex_lock(>tx_cfg_lock);
if (copy_from_user(>tx_cfg, argp,
-  sizeof(struct pi433_tx_cfg)))
+  sizeof(struct pi433_tx_cfg))) {
+   mutex_unlock(>tx_cfg_lock);
return -EFAULT;
+   }
+   mutex_unlock(>tx_cfg_lock);
break;
case PI433_IOC_RD_RX_CFG:
if (copy_to_user(argp, >rx_cfg,
@@ -966,6 +971,8 @@ static int pi433_open(struct inode *inode, struct file 
*filp)
instance->tx_cfg.bit_rate = 4711;
// TODO: fill instance->tx_cfg;
 
+   mutex_init(>tx_cfg_lock);
+
/* instance data as context */
filp->private_data = instance;
nonseekable_open(inode, filp);
-- 
2.17.1
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel