2018-06-02 17:57:36

by Hugo Lefeuvre

[permalink] [raw]
Subject: [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 <[email protected]>
---
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


Attachments:
(No filename) (8.03 kB)
signature.asc (499.00 B)
Download all attachments

2018-06-07 12:46:12

by Hugo Lefeuvre

[permalink] [raw]
Subject: Re: [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 <[email protected]>
> ---
> 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) | http://www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

2018-06-09 08:36:02

by Valentin Vidic

[permalink] [raw]
Subject: Re: [PATCH v2] staging: pi433: add mutex fixing concurrency issues.

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

2018-06-09 15:49:39

by Hugo Lefeuvre

[permalink] [raw]
Subject: Re: [PATCH v2] staging: pi433: add mutex fixing concurrency issues.

> > 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) | http://www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA

2018-06-12 12:12:27

by Dan Carpenter

[permalink] [raw]
Subject: Re: [PATCH v2] staging: pi433: add mutex fixing concurrency issues.

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


2018-06-13 01:16:37

by Hugo Lefeuvre

[permalink] [raw]
Subject: Re: [PATCH v2] staging: pi433: add mutex fixing concurrency issues.

> > 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) | http://www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA


Attachments:
(No filename) (952.00 B)
signature.asc (499.00 B)
Download all attachments