[PATCH 1/3] mfd: ab8500-debugfs: Apply a check for -ENOMEM after allocating memory for sysfs

[PATCH 1/3] mfd: ab8500-debugfs: Apply a check for -ENOMEM after allocating memory for sysfs

[Lee Jones]

The AB8500 debugfs driver allocates memory for a new sysfs entry, but
fails to apply the proper post-allocation checks. If the device were to
run out of memory, the allocation would return NULL. Without the correct
checks the driver will continue to populate NULL->[show|store|...],
which would obviously cause a pointer dereference Oops.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/ab8500-debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c
index 7d1f1b0..c8298b2 100644
--- a/drivers/mfd/ab8500-debugfs.c
+++ b/drivers/mfd/ab8500-debugfs.c
@@ -2800,6 +2800,9 @@ static ssize_t ab8500_subscribe_write(struct file *file,
 	 */
 	dev_attr[irq_index] = kmalloc(sizeof(struct device_attribute),
 		GFP_KERNEL);
+	if (!dev_attr[irq_index])
+		return -ENOMEM;
+
 	event_name[irq_index] = kmalloc(count, GFP_KERNEL);
 	sprintf(event_name[irq_index], "%lu", user_val);
 	dev_attr[irq_index]->show = show_irq;
-- 
1.8.1.2

[PATCH 2/3] mfd: ab8500-debugfs: Apply a check for -ENOMEM after allocating memory for event name

[Lee Jones]

The AB8500 debugfs driver allocates memory to contain the name of a new sysfs
entry, but fails to apply the proper post-allocation checks. If the device
were to run out of memory, the allocation would return NULL. Without the
correct checks the driver will continue to populate address NULL with the
specified device name which would obviously cause a pointer dereference Oops.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/ab8500-debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mfd/ab8500-debugfs.c b/drivers/mfd/ab8500-debugfs.c
index c8298b2..30fcb0c 100644
--- a/drivers/mfd/ab8500-debugfs.c
+++ b/drivers/mfd/ab8500-debugfs.c
@@ -2804,6 +2804,9 @@ static ssize_t ab8500_subscribe_write(struct file *file,
 		return -ENOMEM;
 
 	event_name[irq_index] = kmalloc(count, GFP_KERNEL);
+	if (!event_name[irq_index])
+		return -ENOMEM;
+
 	sprintf(event_name[irq_index], "%lu", user_val);
 	dev_attr[irq_index]->show = show_irq;
 	dev_attr[irq_index]->store = NULL;
-- 
1.8.1.2

[PATCH 3/3] mfd: ucb1x00-core: Rewrite ucb1x00_add_dev()

[Lee Jones]

Error handling is on-its-head in this function. After invoking a function we
should examine the return code and return the error value if there was one.
Instead, this function checks for success and goes onto provide functionality
if success was received. Not so bad in a simple function like this, but in
a more complex one this could end up drowning in curly brackets.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/mfd/ucb1x00-core.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/mfd/ucb1x00-core.c b/drivers/mfd/ucb1x00-core.c
index 70f02da..0690ffd 100644
--- a/drivers/mfd/ucb1x00-core.c
+++ b/drivers/mfd/ucb1x00-core.c
@@ -393,22 +393,24 @@ static struct irq_chip ucb1x00_irqchip = {
 static int ucb1x00_add_dev(struct ucb1x00 *ucb, struct ucb1x00_driver *drv)
 {
 	struct ucb1x00_dev *dev;
-	int ret = -ENOMEM;
+	int ret;
 
 	dev = kmalloc(sizeof(struct ucb1x00_dev), GFP_KERNEL);
-	if (dev) {
-		dev->ucb = ucb;
-		dev->drv = drv;
-
-		ret = drv->add(dev);
-
-		if (ret == 0) {
-			list_add_tail(&dev->dev_node, &ucb->devs);
-			list_add_tail(&dev->drv_node, &drv->devs);
-		} else {
-			kfree(dev);
-		}
+	if (!dev)
+		return -ENOMEM;
+
+	dev->ucb = ucb;
+	dev->drv = drv;
+
+	ret = drv->add(dev);
+	if (ret) {
+		kfree(dev);
+		return ret;
 	}
+
+	list_add_tail(&dev->dev_node, &ucb->devs);
+	list_add_tail(&dev->drv_node, &drv->devs);
+
 	return ret;
 }
 
-- 
1.8.1.2

Re: [PATCH 1/3] mfd: ab8500-debugfs: Apply a check for -ENOMEM after allocating memory for sysfs

[Lee Jones]

On Fri, 19 Jul 2013, Lee Jones wrote:

> The AB8500 debugfs driver allocates memory for a new sysfs entry, but
> fails to apply the proper post-allocation checks. If the device were to
> run out of memory, the allocation would return NULL. Without the correct
> checks the driver will continue to populate NULL->[show|store|...],
> which would obviously cause a pointer dereference Oops.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/mfd/ab8500-debugfs.c | 3 +++
>  1 file changed, 3 insertions(+)

These 3 patches have been on the MLs for quite some time now.

Tentatively applying all 3.

-- 
Lee Jones
Linaro ST-Ericsson Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog