Skip to contentSkip to conversation

mfd: vexpress: use after free in vexpress_syscfg_regmap_init()

PATCH In review Dan Carpenter 4 participants 8 messages Jun 11, 2014 – Jun 13, 2014

Subsystems (3)
Kernel janitors Char and misc drivers Arm/versatile express platform
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 mfd: vexpress: use after free in vexpress_syscfg_regmap_init()

We should return NULL if regmap_init() fails instead of continuing.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +5 -3
index 73068e5..2c0ddb2 100644
@@ -231,10 +231,12 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func->regmap = regmap_init(dev, NULL, func,
 			&vexpress_syscfg_regmap_config);
 
-	if (IS_ERR(func->regmap))
+	if (IS_ERR(func->regmap)) {
 		kfree(func);
-	else
-		list_add(&func->list, &syscfg->funcs);
+		return NULL;
+	}
+
+	list_add(&func->list, &syscfg->funcs);
 
 	return func->regmap;
 }
#1 lore raw
Quoted 14 lines · Dan Carpenter

We should return NULL if regmap_init() fails instead of continuing.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +1 -1
index 73068e5..2c0ddb2 100644
@@ -231,10 +231,12 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func->regmap = regmap_init(dev, NULL, func,
 			&vexpress_syscfg_regmap_config);
 
-	if (IS_ERR(func->regmap))
+	if (IS_ERR(func->regmap)) {

drivers/misc/vexpress-syscfg.c static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, · line 242

Quoted 10 lines
drivers/misc/vexpress-syscfg.c +4 -2
 		kfree(func);
-	else
-		list_add(&func->list, &syscfg->funcs);
+		return NULL;
+	}
+
+	list_add(&func->list, &syscfg->funcs);
 
 	return func->regmap;
 }

Not really, no. What made you think so?

vexpress_config_bridge_ops.regmap_init should return an ERR_PTR in case of troubles, not a NULL. See devm_regmap_init_vexpress_config() in drivers/bus/vexpress-config.c:

        regmap = bridge->ops->regmap_init(dev, bridge->context);
        if (IS_ERR(regmap)) {

Pawel

author
#2 lore raw
Quoted 28 lines · Pawel Moll
Quoted 14 lines · Dan Carpenter

We should return NULL if regmap_init() fails instead of continuing.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +1 -1
index 73068e5..2c0ddb2 100644
@@ -231,10 +231,12 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func->regmap = regmap_init(dev, NULL, func,
 			&vexpress_syscfg_regmap_config);
 
-	if (IS_ERR(func->regmap))
+	if (IS_ERR(func->regmap)) {

drivers/misc/vexpress-syscfg.c static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, · line 242

Quoted 10 lines
drivers/misc/vexpress-syscfg.c +4 -2
 		kfree(func);
-	else
-		list_add(&func->list, &syscfg->funcs);
+		return NULL;
+	}
+
+	list_add(&func->list, &syscfg->funcs);
 
 	return func->regmap;
 }

Not really, no. What made you think so?

Quoted 6 lines

vexpress_config_bridge_ops.regmap_init should return an ERR_PTR in case of troubles, not a NULL. See devm_regmap_init_vexpress_config() in drivers/bus/vexpress-config.c:

        regmap = bridge->ops->regmap_init(dev, bridge->context);
        if (IS_ERR(regmap)) {

Ah... That was sloppy of me. There is a similar bug earlier and I'll fix that as well.

regards,
dan carpenter

author
#3 lore raw

PATCH v2 1/1 mfd: vexpress: fix error handling vexpress_syscfg_regmap_init()

In reply to Pawel Moll

This function should be returning an ERR_PTR() on failure instead of NULL. Also there is a use after free bug if regmap_init() fails because we free "func" and then dereference doing the return.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +8 -4
index 73068e5..3250fc1 100644
@@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func = kzalloc(sizeof(*func) + sizeof(*func->template) * num,
 			GFP_KERNEL);
 	if (!func)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	func->syscfg = syscfg;
 	func->num_templates = num;

@@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func->regmap = regmap_init(dev, NULL, func,
 			&vexpress_syscfg_regmap_config);
 
-	if (IS_ERR(func->regmap))
+	if (IS_ERR(func->regmap)) {
+		void *err = func->regmap;
+
 		kfree(func);
-	else
-		list_add(&func->list, &syscfg->funcs);
+		return err;
+	}
+
+	list_add(&func->list, &syscfg->funcs);
 
 	return func->regmap;
 }
Acked-by on v2 1/1
#4 lore raw

Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init()

Quoted 27 lines · Dan Carpenter

This function should be returning an ERR_PTR() on failure instead of NULL. Also there is a use after free bug if regmap_init() fails because we free "func" and then dereference doing the return.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +4 -2
index 73068e5..3250fc1 100644
@@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func = kzalloc(sizeof(*func) + sizeof(*func->template) * num,
 			GFP_KERNEL);
 	if (!func)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	func->syscfg = syscfg;
 	func->num_templates = num;

@@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func->regmap = regmap_init(dev, NULL, func,
 			&vexpress_syscfg_regmap_config);
 
-	if (IS_ERR(func->regmap))
+	if (IS_ERR(func->regmap)) {
+		void *err = func->regmap;
+

drivers/misc/vexpress-syscfg.c static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, · line 244

Quoted 10 lines
drivers/misc/vexpress-syscfg.c +4 -2
 		kfree(func);
-	else
-		list_add(&func->list, &syscfg->funcs);
+		return err;
+	}
+
+	list_add(&func->list, &syscfg->funcs);
 
 	return func->regmap;
 }

Uh, right. Dereferencing a freed structure. My bad. Thanks for spotting this!

Acked-by Pawel Moll <[email protected]>

(nit: the subject should be "misc: vexpress:" rather then "mfd:")

Arnd, Olof, can you pick this one as an early fix or do you want me to queue it for rc1-based fixes branch?

Paweł

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

maintainer
#5 lore raw

Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init()

Quoted 42 lines · Pawel Moll
Quoted 27 lines · Dan Carpenter

This function should be returning an ERR_PTR() on failure instead of NULL. Also there is a use after free bug if regmap_init() fails because we free "func" and then dereference doing the return.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +4 -2
index 73068e5..3250fc1 100644
@@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func = kzalloc(sizeof(*func) + sizeof(*func->template) * num,
 			GFP_KERNEL);
 	if (!func)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	func->syscfg = syscfg;
 	func->num_templates = num;

@@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
 	func->regmap = regmap_init(dev, NULL, func,
 			&vexpress_syscfg_regmap_config);
 
-	if (IS_ERR(func->regmap))
+	if (IS_ERR(func->regmap)) {
+		void *err = func->regmap;
+

drivers/misc/vexpress-syscfg.c static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, · line 244

Quoted 10 lines
drivers/misc/vexpress-syscfg.c +4 -2
 		kfree(func);
-	else
-		list_add(&func->list, &syscfg->funcs);
+		return err;
+	}
+
+	list_add(&func->list, &syscfg->funcs);
 
 	return func->regmap;
 }

Uh, right. Dereferencing a freed structure. My bad. Thanks for spotting this!

Quoted 7 lines
Acked-by Pawel Moll <[email protected]>

(nit: the subject should be "misc: vexpress:" rather then "mfd:")

Arnd, Olof, can you pick this one as an early fix or do you want me to queue it for rc1-based fixes branch?

I can queue it up.

thanks,

greg k-h

#6 lore raw

Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init()

Quoted 1 line · Greg Kroah-Hartman

I can queue it up.

Cool, thanks!

Pawel

maintainer Applied
#7 lore raw

Re: [patch v2] mfd: vexpress: fix error handling vexpress_syscfg_regmap_init()

In reply to Pawel Moll
Quoted 42 lines · Pawel Moll
Quoted 27 lines · Dan Carpenter

This function should be returning an ERR_PTR() on failure instead of NULL. Also there is a use after free bug if regmap_init() fails because we free "func" and then dereference doing the return.

Signed-off-by Dan Carpenter <[email protected]>
drivers/misc/vexpress-syscfg.c +4 -2
index 73068e5..3250fc1 100644
@@ -199,7 +199,7 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
      func = kzalloc(sizeof(*func) + sizeof(*func->template) * num,
                      GFP_KERNEL);
      if (!func)
-             return NULL;
+             return ERR_PTR(-ENOMEM);
 
      func->syscfg = syscfg;
      func->num_templates = num;

@@ -231,10 +231,14 @@ static struct regmap *vexpress_syscfg_regmap_init(struct device *dev,
      func->regmap = regmap_init(dev, NULL, func,
                      &vexpress_syscfg_regmap_config);
 
-     if (IS_ERR(func->regmap))
+     if (IS_ERR(func->regmap)) {
+             void *err = func->regmap;
+

drivers/misc/vexpress-syscfg.c static struct regmap *vexpress_syscfg_regmap_init(struct device *dev, · line 244

Quoted 10 lines
drivers/misc/vexpress-syscfg.c +4 -2
              kfree(func);
-     else
-             list_add(&func->list, &syscfg->funcs);
+             return err;
+     }
+
+     list_add(&func->list, &syscfg->funcs);
 
      return func->regmap;
 }

Uh, right. Dereferencing a freed structure. My bad. Thanks for spotting this!

Quoted 6 lines
Acked-by Pawel Moll <[email protected]>

(nit: the subject should be "misc: vexpress:" rather then "mfd:")

Arnd, Olof, can you pick this one as an early fix or do you want me to queue it for rc1-based fixes branch?

I've applied it to the fixes branch now. Thanks!

Arnd