[bug report] [media] lirc: prevent use-after free

[bug report] [media] lirc: prevent use-after free

[Dan Carpenter]

Hello Sean Young,

The patch afbb110172b9: "[media] lirc: prevent use-after free" from
Oct 31, 2016, leads to the following static checker warning:

	drivers/media/rc/lirc_dev.c:190 lirc_cdev_add()
	error: potential null dereference 'cdev'.  (cdev_alloc returns null)

drivers/media/rc/lirc_dev.c
   158  static int lirc_cdev_add(struct irctl *ir)
   159  {
   160          int retval = -ENOMEM;
   161          struct lirc_driver *d = &ir->d;
   162          struct cdev *cdev;
   163  
   164          cdev = cdev_alloc();
   165          if (!cdev)
   166                  goto err_out;

Classic one err bug.  Just return directly here.  return -ENOMEM is 100%
readable but goto err_out is opaque because you first have to scroll
down to see what err_out does then you have to scroll to the start of
the function to verify that retval is set.

   167  
   168          if (d->fops) {
   169                  cdev->ops = d->fops;
   170                  cdev->owner = d->owner;
   171          } else {
   172                  cdev->ops = &lirc_dev_fops;
   173                  cdev->owner = THIS_MODULE;
   174          }
   175          retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
   176          if (retval)
   177                  goto err_out;
   178  
   179          retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
   180          if (retval) {
   181                  kobject_put(&cdev->kobj);

This is a double free, isn't it?  It should just be goto del_cdev;

   182                  goto err_out;
   183          }
   184  
   185          ir->cdev = cdev;
   186  
   187          return 0;
   188  
   189  err_out:
   190          cdev_del(cdev);

Can't pass NULLs to this function.

   191          return retval;
   192  }

regards,
dan carpenter

Re: [bug report] [media] lirc: prevent use-after free

[Sean Young]

Hi Dan,

On Sat, Nov 26, 2016 at 12:57:17PM +0300, Dan Carpenter wrote:
> Hello Sean Young,
> 
> The patch afbb110172b9: "[media] lirc: prevent use-after free" from
> Oct 31, 2016, leads to the following static checker warning:
> 
> 	drivers/media/rc/lirc_dev.c:190 lirc_cdev_add()
> 	error: potential null dereference 'cdev'.  (cdev_alloc returns null)
> 
> drivers/media/rc/lirc_dev.c
>    158  static int lirc_cdev_add(struct irctl *ir)
>    159  {
>    160          int retval = -ENOMEM;
>    161          struct lirc_driver *d = &ir->d;
>    162          struct cdev *cdev;
>    163  
>    164          cdev = cdev_alloc();
>    165          if (!cdev)
>    166                  goto err_out;
> 
> Classic one err bug.  Just return directly here.  return -ENOMEM is 100%
> readable but goto err_out is opaque because you first have to scroll
> down to see what err_out does then you have to scroll to the start of
> the function to verify that retval is set.
> 
>    167  
>    168          if (d->fops) {
>    169                  cdev->ops = d->fops;
>    170                  cdev->owner = d->owner;
>    171          } else {
>    172                  cdev->ops = &lirc_dev_fops;
>    173                  cdev->owner = THIS_MODULE;
>    174          }
>    175          retval = kobject_set_name(&cdev->kobj, "lirc%d", d->minor);
>    176          if (retval)
>    177                  goto err_out;
>    178  
>    179          retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
>    180          if (retval) {
>    181                  kobject_put(&cdev->kobj);
> 
> This is a double free, isn't it?  It should just be goto del_cdev;
> 
>    182                  goto err_out;
>    183          }
>    184  
>    185          ir->cdev = cdev;
>    186  
>    187          return 0;
>    188  
>    189  err_out:
>    190          cdev_del(cdev);
> 
> Can't pass NULLs to this function.
> 
>    191          return retval;
>    192  }

Oh dear! Thanks for reporting this, you're absolutely right. I'll send
out a patch shortly.

Thanks
Sean

[PATCH] [media] lirc: fix error paths in lirc_cdev_add()

[Sean Young]

"c77d17c0 [media] lirc: use-after free" introduces two problems:
cdev_del() can be called with a NULL argument, and the kobject_put()
path will cause a double free.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/lirc_dev.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/media/rc/lirc_dev.c b/drivers/media/rc/lirc_dev.c
index d3039ef..3854809 100644
--- a/drivers/media/rc/lirc_dev.c
+++ b/drivers/media/rc/lirc_dev.c
@@ -157,13 +157,13 @@ static const struct file_operations lirc_dev_fops = {
 
 static int lirc_cdev_add(struct irctl *ir)
 {
-	int retval = -ENOMEM;
 	struct lirc_driver *d = &ir->d;
 	struct cdev *cdev;
+	int retval;
 
 	cdev = cdev_alloc();
 	if (!cdev)
-		goto err_out;
+		return -ENOMEM;
 
 	if (d->fops) {
 		cdev->ops = d->fops;
@@ -177,10 +177,8 @@ static int lirc_cdev_add(struct irctl *ir)
 		goto err_out;
 
 	retval = cdev_add(cdev, MKDEV(MAJOR(lirc_base_dev), d->minor), 1);
-	if (retval) {
-		kobject_put(&cdev->kobj);
+	if (retval)
 		goto err_out;
-	}
 
 	ir->cdev = cdev;
 
-- 
2.9.3