Re: [patch] gpio: potential null dereference

[Dan Carpenter <error27@gmail.com>]

On Tue, Apr 27, 2010 at 01:05:15AM +0200, Daniel Glöckner wrote:
> On Mon, Apr 26, 2010 at 09:25:20PM +0200, Dan Carpenter wrote:
> > Smatch found a potential null dereference in gpio_setup_irq().  The 
> > "pdesc" variable is allocated with idr_find() that can return NULL.  If
> > gpio_setup_irq() is called with 0 as gpio_flags and "pdesc" is null, it
> > would OOPs here.
> 
> idr_find() doesn't allocate, idr_get_new_above() does.
> Assuming idr_find() never fails for an id if idr_get_new_above()
> successfully allocated that id, I don't think we can reach that
> line with pdesc being NULL:
> 
> - There are two gotos leading to free_sd
> - #2 is after a block that allocates pdesc
> - #1 is in an if (!gpio_flags) block
> - We exit early if ((desc->flags & GPIO_TRIGGER_MASK) == gpio_flags)
> - Therefore (desc->flags & GPIO_TRIGGER_MASK) must be != 0 to reach #1
> - Trigger flags are added to desc->flags only after we have
>   successfully allocated pdesc (i.e. right before return 0)
> - We start off with no trigger flags set
> 

Are you sure?  If we know that the call to idr_find() returns a valid 
pointer we could remove a lot of error handling code...

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 76be229..54922a6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -330,14 +330,6 @@ static irqreturn_t gpio_sysfs_irq(int irq, void *priv)
 	return IRQ_HANDLED;
 }
 
-static void gpio_notify_sysfs(struct work_struct *work)
-{
-	struct poll_desc	*pdesc;
-
-	pdesc = container_of(work, struct poll_desc, work);
-	sysfs_notify_dirent(pdesc->value_sd);
-}
-
 static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 		unsigned long gpio_flags)
 {
@@ -353,14 +345,10 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 		return -EIO;
 
 	id = desc->flags >> PDESC_ID_SHIFT;
+	/* idr_find() always returns a valid pointer here */
 	pdesc = idr_find(&pdesc_idr, id);
-	if (pdesc) {
-		free_irq(irq, &pdesc->work);
-		cancel_work_sync(&pdesc->work);
-	}
 
 	desc->flags &= ~GPIO_TRIGGER_MASK;
-
 	if (!gpio_flags) {
 		ret = 0;
 		goto free_sd;
@@ -374,39 +362,6 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 		irq_flags |= test_bit(FLAG_ACTIVE_LOW, &desc->flags) ?
 			IRQF_TRIGGER_FALLING : IRQF_TRIGGER_RISING;
 
-	if (!pdesc) {
-		pdesc = kmalloc(sizeof(*pdesc), GFP_KERNEL);
-		if (!pdesc) {
-			ret = -ENOMEM;
-			goto err_out;
-		}
-
-		do {
-			ret = -ENOMEM;
-			if (idr_pre_get(&pdesc_idr, GFP_KERNEL))
-				ret = idr_get_new_above(&pdesc_idr,
-						pdesc, 1, &id);
-		} while (ret == -EAGAIN);
-
-		if (ret)
-			goto free_mem;
-
-		desc->flags &= GPIO_FLAGS_MASK;
-		desc->flags |= (unsigned long)id << PDESC_ID_SHIFT;
-
-		if (desc->flags >> PDESC_ID_SHIFT != id) {
-			ret = -ERANGE;
-			goto free_id;
-		}
-
-		pdesc->value_sd = sysfs_get_dirent(dev->kobj.sd, "value");
-		if (!pdesc->value_sd) {
-			ret = -ENODEV;
-			goto free_id;
-		}
-		INIT_WORK(&pdesc->work, gpio_notify_sysfs);
-	}
-
 	ret = request_irq(irq, gpio_sysfs_irq, irq_flags,
 			"gpiolib", &pdesc->work);
 	if (ret)
@@ -417,12 +372,9 @@ static int gpio_setup_irq(struct gpio_desc *desc, struct device *dev,
 
 free_sd:
 	sysfs_put(pdesc->value_sd);
-free_id:
 	idr_remove(&pdesc_idr, id);
 	desc->flags &= GPIO_FLAGS_MASK;
-free_mem:
 	kfree(pdesc);
-err_out:
 	return ret;
 }