[PATCH v2] staging: pi433: add mutex fixing concurrency issues.
Add a mutex fixing a potential NULL pointer dereference 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 mutex makes sure that instance data will not be modified simultaneously by pi433_release, pi433_write, pi433_read or pi433_ioctl. The mutex is stored in a newly introduced struct pi433_data, which wraps struct pi433_instance and its mutex. Make filp->private_data point to a struct pi433_data, allowing to acquire the lock before accessing the struct pi433_instance. Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com> --- Changes in v2: - Use mutex instead of rw semaphore. - Introduce struct pi433_data in order to allow functions to lock before dereferencing instance pointer. - Make filp->private_data point to a struct pi433_data. - Add missing braces. --- drivers/staging/pi433/pi433_if.c | 77 +++++++++++++++++++++++++------- 1 file changed, 62 insertions(+), 15 deletions(-) diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c index d1e0ddbc79ce..b56dac27e7f1 100644 --- a/drivers/staging/pi433/pi433_if.c +++ b/drivers/staging/pi433/pi433_if.c @@ -118,6 +118,11 @@ struct pi433_instance { struct pi433_tx_cfg tx_cfg; }; +struct pi433_data { + struct pi433_instance *instance; + struct mutex instance_lock; /* guards instance removal */ +}; + /*-------------------------------------------------------------------------*/ /* GPIO interrupt handlers */ @@ -769,6 +774,7 @@ pi433_tx_thread(void *data) static ssize_t pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos) { + struct pi433_data *data; struct pi433_instance *instance; struct pi433_device *device; int bytes_received; @@ -778,13 +784,16 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos) if (size > MAX_MSG_SIZE) return -EMSGSIZE; - instance = filp->private_data; + data = filp->private_data; + mutex_lock(&data->instance_lock); + instance = data->instance; device = instance->device; /* just one read request at a time */ mutex_lock(&device->rx_lock); if (device->rx_active) { mutex_unlock(&device->rx_lock); + mutex_unlock(&data->instance_lock); return -EAGAIN; } @@ -804,10 +813,13 @@ pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos) /* if read was successful copy to user space*/ if (bytes_received > 0) { retval = copy_to_user(buf, device->rx_buffer, bytes_received); - if (retval) + if (retval) { + mutex_unlock(&data->instance_lock); return -EFAULT; + } } + mutex_unlock(&data->instance_lock); return bytes_received; } @@ -815,17 +827,22 @@ static ssize_t pi433_write(struct file *filp, const char __user *buf, size_t count, loff_t *f_pos) { + struct pi433_data *data; struct pi433_instance *instance; struct pi433_device *device; int retval; unsigned int copied; - instance = filp->private_data; + data = filp->private_data; + mutex_lock(&data->instance_lock); + instance = data->instance; device = instance->device; /* check, whether internal buffer (tx thread) is big enough for requested size */ - if (count > MAX_MSG_SIZE) + if (count > MAX_MSG_SIZE) { + mutex_unlock(&data->instance_lock); return -EMSGSIZE; + } /* write the following sequence into fifo: * - tx_cfg @@ -851,6 +868,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); + mutex_unlock(&data->instance_lock); return copied; @@ -858,6 +876,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); + mutex_unlock(&data->instance_lock); return -EAGAIN; } @@ -865,6 +884,7 @@ static long pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { int retval = 0; + struct pi433_data *data; struct pi433_instance *instance; struct pi433_device *device; void __user *argp = (void __user *)arg; @@ -873,30 +893,37 @@ 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() - */ - instance = filp->private_data; + data = filp->private_data; + mutex_lock(&data->instance_lock); + instance = data->instance; device = instance->device; - if (!device) + if (!device) { + mutex_unlock(&data->instance_lock); return -ESHUTDOWN; + } switch (cmd) { case PI433_IOC_RD_TX_CFG: if (copy_to_user(argp, &instance->tx_cfg, - sizeof(struct pi433_tx_cfg))) + sizeof(struct pi433_tx_cfg))) { + mutex_unlock(&data->instance_lock); return -EFAULT; + } break; case PI433_IOC_WR_TX_CFG: if (copy_from_user(&instance->tx_cfg, argp, - sizeof(struct pi433_tx_cfg))) + sizeof(struct pi433_tx_cfg))) { + mutex_unlock(&data->instance_lock); return -EFAULT; + } break; case PI433_IOC_RD_RX_CFG: if (copy_to_user(argp, &device->rx_cfg, - sizeof(struct pi433_rx_cfg))) + sizeof(struct pi433_rx_cfg))) { + mutex_unlock(&data->instance_lock); return -EFAULT; + } break; case PI433_IOC_WR_RX_CFG: mutex_lock(&device->rx_lock); @@ -904,21 +931,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); + mutex_unlock(&data->instance_lock); return -EAGAIN; } if (copy_from_user(&device->rx_cfg, argp, sizeof(struct pi433_rx_cfg))) { mutex_unlock(&device->rx_lock); + mutex_unlock(&data->instance_lock); return -EFAULT; } mutex_unlock(&device->rx_lock); + mutex_unlock(&data->instance_lock); break; default: + mutex_unlock(&data->instance_lock); retval = -EINVAL; } + mutex_unlock(&data->instance_lock); return retval; } @@ -936,6 +968,7 @@ pi433_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) static int pi433_open(struct inode *inode, struct file *filp) { + struct pi433_data *data; struct pi433_device *device; struct pi433_instance *instance; @@ -954,20 +987,30 @@ static int pi433_open(struct inode *inode, struct file *filp) } device->users++; + data = kzalloc(sizeof(*data), GFP_KERNEL); + if (!data) { + kfree(device->rx_buffer); + device->rx_buffer = NULL; + return -ENOMEM; + } + mutex_init(&data->instance_lock); + instance = kzalloc(sizeof(*instance), GFP_KERNEL); if (!instance) { + kfree(data); kfree(device->rx_buffer); device->rx_buffer = NULL; return -ENOMEM; } /* setup instance data*/ + data->instance = instance; instance->device = device; instance->tx_cfg.bit_rate = 4711; // TODO: fill instance->tx_cfg; - /* instance data as context */ - filp->private_data = instance; + /* instance data wrapper as context */ + filp->private_data = data; nonseekable_open(inode, filp); return 0; @@ -975,12 +1018,16 @@ static int pi433_open(struct inode *inode, struct file *filp) static int pi433_release(struct inode *inode, struct file *filp) { + struct pi433_data *data; struct pi433_instance *instance; struct pi433_device *device; - instance = filp->private_data; + data = filp->private_data; + mutex_lock(&data->instance_lock); + instance = data->instance; device = instance->device; kfree(instance); + kfree(data); filp->private_data = NULL; /* last close? */ -- 2.17.1
Add a mutex fixing a potential NULL pointer dereference 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 mutex makes sure that instance data will not be modified simultaneously by pi433_release, pi433_write, pi433_read or pi433_ioctl.
The mutex is stored in a newly introduced struct pi433_data, which wraps struct pi433_instance and its mutex.
Make filp->private_data point to a struct pi433_data, allowing to acquire the lock before accessing the struct pi433_instance.
Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com> --- Changes in v2: - Use mutex instead of rw semaphore. - Introduce struct pi433_data in order to allow functions to lock before dereferencing instance pointer. - Make filp->private_data point to a struct pi433_data. - Add missing braces.
After discussing this issue on the kernel newbies mailing list[0] we came to the conclusion that it is very unlikely that pi433_release and pi433_ioctl would ever run concurrently in this case. This is also true for read/write. Unless one can find a situation where this might happen, I think we should not add this potentially unnecessary lock. Regards, Hugo [0] http://lists.kernelnewbies.org/pipermail/kernelnewbies/2018-June/019131.html -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Thu, Jun 07, 2018 at 08:45:03AM -0400, Hugo Lefeuvre wrote:
After discussing this issue on the kernel newbies mailing list[0] we came to the conclusion that it is very unlikely that pi433_release and pi433_ioctl would ever run concurrently in this case. This is also true for read/write. Unless one can find a situation where this might happen, I think we should not add this potentially unnecessary lock.
Yes, so we should than drop the TODO comment on this issue? -- Valentin
After discussing this issue on the kernel newbies mailing list[0] we came to the conclusion that it is very unlikely that pi433_release and pi433_ioctl would ever run concurrently in this case. This is also true for read/write. Unless one can find a situation where this might happen, I think we should not add this potentially unnecessary lock.
Yes, so we should than drop the TODO comment on this issue?
Well, after taking a closer look it appears that I misunderstood the TODO. What this TODO means is that we might run into a whole world of issues if the device is _removed_ while we're running ioctl or I guess pretty much any function that accesses the struct pi433_device. So the issue doesn't come from pi433_release and pi433_ioctl running concurrently, but rather pi433_ioctl and pi433_remove. Whether this situation is likely to happen or not is another question which I am currently taking a look at. Also, during my work on this driver I found what I'd consider as another issue: In pi433_ioctl we execute case PI433_IOC_WR_TX_CFG: if (copy_from_user(&instance->tx_cfg, argp, sizeof(struct pi433_tx_cfg))) return -EFAULT; break; without any synchronization. What if two ioctl syscalls are called with command PI433_IOC_WR_TX_CFG ? instance->tx_cfg might get corrupt unless copy_from_user provides some kind of synchronization, right ? I have prepared a patch but couldn't test it because I don't have test devices. -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Sat, Jun 09, 2018 at 11:48:42AM -0400, Hugo Lefeuvre wrote:
case PI433_IOC_WR_TX_CFG: if (copy_from_user(&instance->tx_cfg, argp, sizeof(struct pi433_tx_cfg))) return -EFAULT; break;
Btw, it looks so wrong to me that we copy partial data to &instance->tx_cfg... I'd really prefer copying it to a tmp buffer and then verifying it's corrent then memcpy()ing it to &instance->tx_cfg. regards, dan carpenter
case PI433_IOC_WR_TX_CFG: if (copy_from_user(&instance->tx_cfg, argp, sizeof(struct pi433_tx_cfg))) return -EFAULT; break;
Btw, it looks so wrong to me that we copy partial data to &instance->tx_cfg... I'd really prefer copying it to a tmp buffer and then verifying it's corrent then memcpy()ing it to &instance->tx_cfg.
AFAIK this piece of code is not supposed to check passed tx config. These checks are realised later by rf69_set_tx_cfg (called by pi433_tx_thread) after the config has been written to the fifo by pi433_write. What kind of checks do you want to perform exactly ? But, right, I prefer the idea of the temporary buffer too, and seeing the rest of kernel code it seems to be the usual way to go. Regards, Hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
participants (3)
-
Dan Carpenter -
Hugo Lefeuvre -
Valentin Vidic