[PATCH] staging: pi433: add mutex fixing race condition when accessing tx_cfg
In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is modified using copy_from_user(&instance->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 <hle@owl.eu.com> --- 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 mutex tx_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(&instance->tx_cfg_lock); if (copy_from_user(&instance->tx_cfg, argp, - sizeof(struct pi433_tx_cfg))) + sizeof(struct pi433_tx_cfg))) { + mutex_unlock(&instance->tx_cfg_lock); return -EFAULT; + } + mutex_unlock(&instance->tx_cfg_lock); break; case PI433_IOC_RD_RX_CFG: if (copy_to_user(argp, &device->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(&instance->tx_cfg_lock); + /* instance data as context */ filp->private_data = instance; nonseekable_open(inode, filp); -- 2.17.1
On Mon, Jun 11, 2018 at 09:12:00PM -0400, Hugo Lefeuvre wrote:
In the PI433_IOC_WR_TX_CFG case in pi433_ioctl, instance->tx_cfg is modified using
copy_from_user(&instance->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 <hle@owl.eu.com>
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. This bug could cause a user space program to fail. regards, dan carpenter
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
participants (2)
-
Dan Carpenter -
Hugo Lefeuvre