[PATCH] staging: pi433: fix race condition in pi433_open

[PATCH] staging: pi433: fix race condition in pi433_open

[Hugo Lefeuvre]

Whenever pi433_open and pi433_remove execute concurrently, a race
condition potentially resulting in use-after-free might happen.

Let T1 and T2 be two kernel threads.

1. T1 executes pi433_open and stops before "device->users++".
2. The pi433 device was removed inbetween, so T2 executes pi433_remove
   and frees device because the user count has not been incremented yet.
3. T1 executes "device->users++" (use-after-free).

This race condition happens because the check of minor number and
user count increment does not happen atomically.

Fix: Extend scope of minor_lock in pi433_open().

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

diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
index 94e0bfcec991..73c511249f7f 100644
--- a/drivers/staging/pi433/pi433_if.c
+++ b/drivers/staging/pi433/pi433_if.c
@@ -957,11 +957,13 @@ static int pi433_open(struct inode *inode, struct file *filp)
 
 	mutex_lock(&minor_lock);
 	device = idr_find(&pi433_idr, iminor(inode));
-	mutex_unlock(&minor_lock);
 	if (!device) {
+		mutex_unlock(&minor_lock);
 		pr_debug("device: minor %d unknown.\n", iminor(inode));
 		return -ENODEV;
 	}
+	device->users++;
+	mutex_unlock(&minor_lock);
 
 	if (!device->rx_buffer) {
 		device->rx_buffer = kmalloc(MAX_MSG_SIZE, GFP_KERNEL);
@@ -969,7 +971,6 @@ static int pi433_open(struct inode *inode, struct file *filp)
 			return -ENOMEM;
 	}
 
-	device->users++;
 	instance = kzalloc(sizeof(*instance), GFP_KERNEL);
 	if (!instance) {
 		kfree(device->rx_buffer);
-- 
2.17.1

Re: [PATCH] staging: pi433: fix race condition in pi433_open

[Dan Carpenter]

On Sun, Jun 17, 2018 at 10:24:00PM -0400, Hugo Lefeuvre wrote:
> Whenever pi433_open and pi433_remove execute concurrently, a race
> condition potentially resulting in use-after-free might happen.
> 
> Let T1 and T2 be two kernel threads.
> 
> 1. T1 executes pi433_open and stops before "device->users++".
> 2. The pi433 device was removed inbetween, so T2 executes pi433_remove
>    and frees device because the user count has not been incremented yet.
> 3. T1 executes "device->users++" (use-after-free).
> 
> This race condition happens because the check of minor number and
> user count increment does not happen atomically.
> 
> Fix: Extend scope of minor_lock in pi433_open().
> 
> Signed-off-by: Hugo Lefeuvre <hle@owl.eu.com>
> ---
>  drivers/staging/pi433/pi433_if.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
> index 94e0bfcec991..73c511249f7f 100644
> --- a/drivers/staging/pi433/pi433_if.c
> +++ b/drivers/staging/pi433/pi433_if.c
> @@ -957,11 +957,13 @@ static int pi433_open(struct inode *inode, struct file *filp)
>  
>  	mutex_lock(&minor_lock);
>  	device = idr_find(&pi433_idr, iminor(inode));
> -	mutex_unlock(&minor_lock);
>  	if (!device) {
> +		mutex_unlock(&minor_lock);
>  		pr_debug("device: minor %d unknown.\n", iminor(inode));
>  		return -ENODEV;
>  	}
> +	device->users++;
> +	mutex_unlock(&minor_lock);
>  
>  	if (!device->rx_buffer) {
>  		device->rx_buffer = kmalloc(MAX_MSG_SIZE, GFP_KERNEL);

We need to decrement device->users-- on the error paths as well.
This function was already slightly broken with respect to counting the
users, but let's not make it worse.

I think it's still a tiny bit racy because it's not an atomic type.

I'm not sure the error handling in open() works either.  It's releasing
device->rx_buffer but there could be another user.  The ->rx_buffer
should be allocated in probe() instead of open() probably, no?  And then
freed in pi433_remove().  Then once we put that in the right layer
it means we can just get rid of ->users...

The lines:

  1008                  if (!device->spi)
  1009                          kfree(device);

make no sort of sense at all...  Fortunately it's not posssible for
device->spi to be NULL so it's dead code.

regards,
dan carpenter

Re: [PATCH] staging: pi433: fix race condition in pi433_open

[Hugo Lefeuvre]

Hi Dan,

> We need to decrement device->users-- on the error paths as well.
> This function was already slightly broken with respect to counting the
> users, but let's not make it worse.
> 
> I think it's still a tiny bit racy because it's not an atomic type.

Oh right, I missed that. I'll fix it in the v2. :)

> I'm not sure the error handling in open() works either.  It's releasing
> device->rx_buffer but there could be another user.

Agree.

> The ->rx_buffer
> should be allocated in probe() instead of open() probably, no?  And then
> freed in pi433_remove().  Then once we put that in the right layer
> it means we can just get rid of ->users...

It would be great to get rid of this counter, indeed. But how to do it
properly without breaking things ? It seems to be useful to me...

For example, how do you handle the case where remove() is called but
some operations are still running on existing fds ?

What if remove frees the rx_buffer while a read() call executes this ?

copy_to_user(buf, device->rx_buffer, bytes_received)

rx_buffer is freed by release() because it's the only buffer from the
device structure used in read/write/ioctl, meaning that we can only
free it when we are sure that it isn't used anywhere anymore.

So, we can't do it in remove() unless remove() is delayed until the
last release() has returned.

> The lines:
>
>   1008                  if (!device->spi)
>   1009                          kfree(device);
>
> make no sort of sense at all...  Fortunately it's not posssible for
> device->spi to be NULL so it's dead code.

Really ? device->spi is NULL-ed in remove() so that operations on
remaining fds can detect remove() was already called and free remaining
resources:
 
1296         /* make sure ops on existing fds can abort cleanly */
1297         device->spi = NULL;

Thanks for your time !

Regards,
 Hugo

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

Re: [PATCH] staging: pi433: fix race condition in pi433_open

[Dan Carpenter]

On Mon, Jun 18, 2018 at 11:11:36PM -0400, Hugo Lefeuvre wrote:
> Hi Dan,
> 
> > We need to decrement device->users-- on the error paths as well.
> > This function was already slightly broken with respect to counting the
> > users, but let's not make it worse.
> > 
> > I think it's still a tiny bit racy because it's not an atomic type.
> 
> Oh right, I missed that. I'll fix it in the v2. :)
> 
> > I'm not sure the error handling in open() works either.  It's releasing
> > device->rx_buffer but there could be another user.
> 
> Agree.
> 
> > The ->rx_buffer
> > should be allocated in probe() instead of open() probably, no?  And then
> > freed in pi433_remove().  Then once we put that in the right layer
> > it means we can just get rid of ->users...
> 
> It would be great to get rid of this counter, indeed. But how to do it
> properly without breaking things ? It seems to be useful to me...

These things are refcounted so you can't unload the module while a file
is open.  When we do an open it does a cdev_get().  When we call the
delete_module syscall, it checks the refcount in try_stop_module() ->
try_release_module_ref().  It will still let us force a release but
at that point you're expecting use after frees.

> 
> For example, how do you handle the case where remove() is called but
> some operations are still running on existing fds ?
> 
> What if remove frees the rx_buffer while a read() call executes this ?
> 
> copy_to_user(buf, device->rx_buffer, bytes_received)
> 
> rx_buffer is freed by release() because it's the only buffer from the
> device structure used in read/write/ioctl, meaning that we can only
> free it when we are sure that it isn't used anywhere anymore.
> 
> So, we can't do it in remove() unless remove() is delayed until the
> last release() has returned.
> 
> > The lines:
> >
> >   1008                  if (!device->spi)
> >   1009                          kfree(device);
> >
> > make no sort of sense at all...  Fortunately it's not posssible for
> > device->spi to be NULL so it's dead code.
> 
> Really ? device->spi is NULL-ed in remove() so that operations on
> remaining fds can detect remove() was already called and free remaining
> resources:
>  
> 1296         /* make sure ops on existing fds can abort cleanly */
> 1297         device->spi = NULL;
> 
> Thanks for your time !

That's when we're unloading the module so there aren't any users left.

regards,
dan carpenter

Re: [PATCH] staging: pi433: fix race condition in pi433_open

[Hugo Lefeuvre]

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

> > It would be great to get rid of this counter, indeed. But how to do it
> > properly without breaking things ? It seems to be useful to me...
> 
> These things are refcounted so you can't unload the module while a file
> is open.  When we do an open it does a cdev_get().  When we call the
> delete_module syscall, it checks the refcount in try_stop_module() ->
> try_release_module_ref().  It will still let us force a release but
> at that point you're expecting use after frees.

Then I'd like to understand why this counter was introduced if remove
always gets executed after the last release call returned. This was
probably unclear to the original developer.

This TODO seems to be related to this misunderstanding too:

 890         /* TODO? guard against device removal before, or while,
 891          * we issue this ioctl. --> device_get()
 892          */

Device removal can't happen before or during the ioctl execution.

> > Really ? device->spi is NULL-ed in remove() so that operations on
> > remaining fds can detect remove() was already called and free remaining
> > resources:
> >  
> > 1296         /* make sure ops on existing fds can abort cleanly */
> > 1297         device->spi = NULL;
> 
> That's when we're unloading the module so there aren't any users left.

I'll submit an updated version of my patch getting rid of the counter
and addressing the remaining race conditions.

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 --]

Re: [PATCH] staging: pi433: fix race condition in pi433_open

[Dan Carpenter]

On Tue, Jun 19, 2018 at 09:42:20AM -0400, Hugo Lefeuvre wrote:
> This TODO seems to be related to this misunderstanding too:
> 
>  890         /* TODO? guard against device removal before, or while,
>  891          * we issue this ioctl. --> device_get()
>  892          */
> 
> Device removal can't happen before or during the ioctl execution.

Correct.  Just delete the comment.

regards,
dan carpenter