[PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[H Hartley Sweeten]

During the probe for physmap platform flash devices there are a
number error exit conditions that all do a goto err_out which
then calls physmap_flash_remove().  In that function one of the
cleanup steps is:

#ifdef CONFIG_MTD_CONCAT
	if (info->cmtd != info->mtd[0])
		mtd_concat_destroy(info->cmtd);
#endif

This test will succeed since info->cmtd == NULL and info->mtd[0] is
valid.

Fix this by exiting the remove function when info->cmtd == NULL.

Also, cleanup the #ifdef CONFIG_MTD_PARTITIONS stuff by using
mtd_has_partitions().

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: David Woodhouse <dwmw2@infradead.org>

---

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 380648e..65f52d4 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -44,22 +44,23 @@ static int physmap_flash_remove(struct platform_device *dev)
 		return 0;
 	platform_set_drvdata(dev, NULL);
 
+	if (info->cmtd == NULL)
+		return 0;
+
 	physmap_data = dev->dev.platform_data;
 
-	if (info->cmtd) {
-#ifdef CONFIG_MTD_PARTITIONS
-		if (info->nr_parts || physmap_data->nr_parts)
+	if (mtd_has_partitions()) {
+		if (info->nr_parts || physmap_data->nr_parts) {
 			del_mtd_partitions(info->cmtd);
-		else
+
+			if (info->nr_parts)
+				kfree(info->parts);
+		} else {
 			del_mtd_device(info->cmtd);
-#else
+		}
+	} else {
 		del_mtd_device(info->cmtd);
-#endif
 	}
-#ifdef CONFIG_MTD_PARTITIONS
-	if (info->nr_parts)
-		kfree(info->parts);
-#endif
 
 #ifdef CONFIG_MTD_CONCAT
 	if (info->cmtd != info->mtd[0])
@@ -169,22 +170,22 @@ static int physmap_flash_probe(struct platform_device *dev)
 	if (err)
 		goto err_out;
 
-#ifdef CONFIG_MTD_PARTITIONS
-	err = parse_mtd_partitions(info->cmtd, part_probe_types,
-				&info->parts, 0);
-	if (err > 0) {
-		add_mtd_partitions(info->cmtd, info->parts, err);
-		info->nr_parts = err;
-		return 0;
-	}
+	if (mtd_has_partitions()) {
+		err = parse_mtd_partitions(info->cmtd, part_probe_types,
+					&info->parts, 0);
+		if (err > 0) {
+			add_mtd_partitions(info->cmtd, info->parts, err);
+			info->nr_parts = err;
+			return 0;
+		}
 
-	if (physmap_data->nr_parts) {
-		printk(KERN_NOTICE "Using physmap partition information\n");
-		add_mtd_partitions(info->cmtd, physmap_data->parts,
-				   physmap_data->nr_parts);
-		return 0;
+		if (physmap_data->nr_parts) {
+			printk(KERN_NOTICE "Using physmap partition information\n");
+			add_mtd_partitions(info->cmtd, physmap_data->parts,
+					physmap_data->nr_parts);
+			return 0;
+		}
 	}
-#endif
 
 	add_mtd_device(info->cmtd);
 	return 0; 

Re: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[Atsushi Nemoto]

On Mon, 19 Oct 2009 13:31:46 -0400, "H Hartley Sweeten" <hartleys@visionengravers.com> wrote:
> During the probe for physmap platform flash devices there are a
> number error exit conditions that all do a goto err_out which
> then calls physmap_flash_remove().  In that function one of the
> cleanup steps is:
> 
> #ifdef CONFIG_MTD_CONCAT
> 	if (info->cmtd != info->mtd[0])
> 		mtd_concat_destroy(info->cmtd);
> #endif
> 
> This test will succeed since info->cmtd == NULL and info->mtd[0] is
> valid.

Oh I had missed this case when fixing physmap_flash_remove last time.

> Fix this by exiting the remove function when info->cmtd == NULL.

No, map_destroy loop at the end of the function should not be skipped
even when info->cmtd == NULL.

> Also, cleanup the #ifdef CONFIG_MTD_PARTITIONS stuff by using
> mtd_has_partitions().

And this cleanup cause build errors when CONFIG_MTD_PARTITIONS was not
set.  A separate patch might be better for such cleanup.

---
Atsushi Nemoto

RE: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[H Hartley Sweeten]

On Tuesday, October 20, 2009 8:30 AM, Atsushi Nemoto wrote:
> On Mon, 19 Oct 2009 13:31:46 -0400, "H Hartley Sweeten" <hartleys@visionengravers.com> wrote:
>> During the probe for physmap platform flash devices there are a
>> number error exit conditions that all do a goto err_out which
>> then calls physmap_flash_remove().  In that function one of the
>> cleanup steps is:
>> 
>> #ifdef CONFIG_MTD_CONCAT
>> 	if (info->cmtd != info->mtd[0])
>> 		mtd_concat_destroy(info->cmtd);
>> #endif
>> 
>> This test will succeed since info->cmtd == NULL and info->mtd[0] is
>> valid.
>
> Oh I had missed this case when fixing physmap_flash_remove last time.
>
>> Fix this by exiting the remove function when info->cmtd == NULL.
>
> No, map_destroy loop at the end of the function should not be skipped
> even when info->cmtd == NULL.

Missed that part.  I will modify the patch and repost.

>> Also, cleanup the #ifdef CONFIG_MTD_PARTITIONS stuff by using
>> mtd_has_partitions().
>
> And this cleanup cause build errors when CONFIG_MTD_PARTITIONS was not
> set.  A separate patch might be better for such cleanup.

Hmm..  Not sure why that would cause a build error.  Regardless, I will
remove that change from this patch.

Regards,
Hartley

Re: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[Atsushi Nemoto]

On Tue, 20 Oct 2009 12:08:00 -0400, "H Hartley Sweeten" <hartleys@visionengravers.com> wrote:
> >> Also, cleanup the #ifdef CONFIG_MTD_PARTITIONS stuff by using
> >> mtd_has_partitions().
> >
> > And this cleanup cause build errors when CONFIG_MTD_PARTITIONS was not
> > set.  A separate patch might be better for such cleanup.
> 
> Hmm..  Not sure why that would cause a build error.  Regardless, I will
> remove that change from this patch.

Thank you.  The build errors are something like:

physmap.c:53: error: 'struct physmap_flash_info' has no member named 'nr_parts'
physmap.c:174: error: 'part_probe_types' undeclared (first use in this function)

---
Atsushi Nemoto

RE: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[H Hartley Sweeten]

On Tuesday, October 20, 2009 9:18 AM, Atsushi Nemoto wrote:
> On Tue, 20 Oct 2009 12:08:00 -0400, "H Hartley Sweeten" <hartleys@visionengravers.com> wrote:
>>>> Also, cleanup the #ifdef CONFIG_MTD_PARTITIONS stuff by using
>>>> mtd_has_partitions().
>>>
>>> And this cleanup cause build errors when CONFIG_MTD_PARTITIONS was not
>>> set.  A separate patch might be better for such cleanup.
>> 
>> Hmm..  Not sure why that would cause a build error.  Regardless, I will
>> remove that change from this patch.
>
> Thank you.  The build errors are something like:
>
> physmap.c:53: error: 'struct physmap_flash_info' has no member named 'nr_parts'
> physmap.c:174: error: 'part_probe_types' undeclared (first use in this function)

Ok.  That makes sense.

struct physmap_flash_info {
	struct mtd_info		*mtd[MAX_RESOURCES];
	struct mtd_info		*cmtd;
	struct map_info		map[MAX_RESOURCES];
#ifdef CONFIG_MTD_PARTITIONS
	int			nr_parts;
	struct mtd_partition	*parts;
#endif
};

#ifdef CONFIG_MTD_PARTITIONS
static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
#endif

I assumed that mtd_has_partitions() when CONFIG_MTD_PARTITIONS was not defined
would end up as something like this when compiled:

	if (mtd_has_partitions()) {
		/* ... */
	}

Would become:

	if (0) {
		/* ... */
	}

Then it would just optimze away.  It appears the code in the if (0) condition is
still parsed by the compiler so you get the errors above since the symbols are
#ifdef'ed out.

Oh well... I did post the updated patch based on your other comment.

Regards,
Hartley

RE: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[H Hartley Sweeten]

During the probe for physmap platform flash devices there are a
number error exit conditions that all do a goto err_out which
then calls physmap_flash_remove().  In that function one of the
cleanup steps is:

#ifdef CONFIG_MTD_CONCAT
	if (info->cmtd != info->mtd[0])
		mtd_concat_destroy(info->cmtd);
#endif

This test will succeed since info->cmtd == NULL and info->mtd[0] is
valid, which then causes a NULL pointer dereference when mtd_concat_destroy()
is called.  Fix this by moving the mtd_concat_destroy() step into the
if (info->cmtd) condition.

Also, move the kfree(info->parts) cleanup to remove an #ifdef.

Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

---

V2 - As pointed out by Atsushi Nemoto, the map_destroy loop should not
     be skipped even when info->cmtd == NULL.

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 380648e..3f13a96 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -48,23 +48,22 @@ static int physmap_flash_remove(struct platform_device *dev)
 
 	if (info->cmtd) {
 #ifdef CONFIG_MTD_PARTITIONS
-		if (info->nr_parts || physmap_data->nr_parts)
+		if (info->nr_parts || physmap_data->nr_parts) {
 			del_mtd_partitions(info->cmtd);
-		else
+
+			if (info->nr_parts)
+				kfree(info->parts);
+		} else {
 			del_mtd_device(info->cmtd);
+		}
 #else
 		del_mtd_device(info->cmtd);
 #endif
-	}
-#ifdef CONFIG_MTD_PARTITIONS
-	if (info->nr_parts)
-		kfree(info->parts);
-#endif
-
 #ifdef CONFIG_MTD_CONCAT
-	if (info->cmtd != info->mtd[0])
-		mtd_concat_destroy(info->cmtd);
+		if (info->cmtd != info->mtd[0])
+			mtd_concat_destroy(info->cmtd);
 #endif
+	}
 
 	for (i = 0; i < MAX_RESOURCES; i++) {
 		if (info->mtd[i] != NULL) 

RE: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[David Woodhouse]

On Tue, 2009-10-20 at 12:23 -0400, H Hartley Sweeten wrote:
> During the probe for physmap platform flash devices there are a
> number error exit conditions that all do a goto err_out which
> then calls physmap_flash_remove().  In that function one of the
> cleanup steps is:
> 
> #ifdef CONFIG_MTD_CONCAT
> 	if (info->cmtd != info->mtd[0])
> 		mtd_concat_destroy(info->cmtd);
> #endif
> 
> This test will succeed since info->cmtd == NULL and info->mtd[0] is
> valid, which then causes a NULL pointer dereference when mtd_concat_destroy()
> is called.  Fix this by moving the mtd_concat_destroy() step into the
> if (info->cmtd) condition.
> 
> Also, move the kfree(info->parts) cleanup to remove an #ifdef.
> 
> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> ---
> 
> V2 - As pointed out by Atsushi Nemoto, the map_destroy loop should not
>      be skipped even when info->cmtd == NULL.

Thanks.

In an attempt to improve my responsiveness as maintainer, I'd already
committed the first version. How does this look:

commit 8ce110ac19bc88b82e3feacfbb3a2ee08a07fe22
Author: H Hartley Sweeten <hartleys@visionengravers.com>
Date:   Tue Oct 20 12:23:33 2009 -0400

    mtd: Fix compile failure and error path in physmap.c
    
    Commit 4b56ffcacee937a85bf39e14872dd141e23ee85f ("mtd: Fix kernel NULL
    pointer dereference in physmap.c") introduced a couple of bugs.
    
    It neglected to run the loop of map_destroy() calls in
    physmap_flash_remove(), if !info->cmtd, which would happen if that
    function was called to clean up errors during probe.
    
    It also failed to compile if CONFIG_MTD_PARTITIONS was not defined.
    
    Reported-By: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
    Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
    Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 65f52d4..3f13a96 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -44,12 +44,10 @@ static int physmap_flash_remove(struct platform_device *dev)
 		return 0;
 	platform_set_drvdata(dev, NULL);
 
-	if (info->cmtd == NULL)
-		return 0;
-
 	physmap_data = dev->dev.platform_data;
 
-	if (mtd_has_partitions()) {
+	if (info->cmtd) {
+#ifdef CONFIG_MTD_PARTITIONS
 		if (info->nr_parts || physmap_data->nr_parts) {
 			del_mtd_partitions(info->cmtd);
 
@@ -58,14 +56,14 @@ static int physmap_flash_remove(struct platform_device *dev)
 		} else {
 			del_mtd_device(info->cmtd);
 		}
-	} else {
+#else
 		del_mtd_device(info->cmtd);
-	}
-
+#endif
 #ifdef CONFIG_MTD_CONCAT
-	if (info->cmtd != info->mtd[0])
-		mtd_concat_destroy(info->cmtd);
+		if (info->cmtd != info->mtd[0])
+			mtd_concat_destroy(info->cmtd);
 #endif
+	}
 
 	for (i = 0; i < MAX_RESOURCES; i++) {
 		if (info->mtd[i] != NULL)
@@ -170,22 +168,22 @@ static int physmap_flash_probe(struct platform_device *dev)
 	if (err)
 		goto err_out;
 
-	if (mtd_has_partitions()) {
-		err = parse_mtd_partitions(info->cmtd, part_probe_types,
-					&info->parts, 0);
-		if (err > 0) {
-			add_mtd_partitions(info->cmtd, info->parts, err);
-			info->nr_parts = err;
-			return 0;
-		}
+#ifdef CONFIG_MTD_PARTITIONS
+	err = parse_mtd_partitions(info->cmtd, part_probe_types,
+				&info->parts, 0);
+	if (err > 0) {
+		add_mtd_partitions(info->cmtd, info->parts, err);
+		info->nr_parts = err;
+		return 0;
+	}
 
-		if (physmap_data->nr_parts) {
-			printk(KERN_NOTICE "Using physmap partition information\n");
-			add_mtd_partitions(info->cmtd, physmap_data->parts,
-					physmap_data->nr_parts);
-			return 0;
-		}
+	if (physmap_data->nr_parts) {
+		printk(KERN_NOTICE "Using physmap partition information\n");
+		add_mtd_partitions(info->cmtd, physmap_data->parts,
+				   physmap_data->nr_parts);
+		return 0;
 	}
+#endif
 
 	add_mtd_device(info->cmtd);
 	return 0;


-- 
dwmw2

RE: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[H Hartley Sweeten]

On Tuesday, October 20, 2009 2:38 PM, David Woodhouse wrote:
> On Tue, 2009-10-20 at 12:23 -0400, H Hartley Sweeten wrote:
>> During the probe for physmap platform flash devices there are a
>> number error exit conditions that all do a goto err_out which
>> then calls physmap_flash_remove().  In that function one of the
>> cleanup steps is:
>> 
>> #ifdef CONFIG_MTD_CONCAT
>> 	if (info->cmtd != info->mtd[0])
>> 		mtd_concat_destroy(info->cmtd);
>> #endif
>> 
>> This test will succeed since info->cmtd == NULL and info->mtd[0] is
>> valid, which then causes a NULL pointer dereference when mtd_concat_destroy()
>> is called.  Fix this by moving the mtd_concat_destroy() step into the
>> if (info->cmtd) condition.
>> 
>> Also, move the kfree(info->parts) cleanup to remove an #ifdef.
>> 
>> Signed-off-by: H Hartley Sweeten <hsweeten@visionengravers.com>
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>> 
>> ---
>> 
>> V2 - As pointed out by Atsushi Nemoto, the map_destroy loop should not
>>      be skipped even when info->cmtd == NULL.
>
> Thanks.
>
> In an attempt to improve my responsiveness as maintainer, I'd already
> committed the first version. How does this look:

Very responsive indeed.  ;-)

Sorry about introducing the bug.  Your amended patch looks like it serves
the same purpose as my updated one.  Thanks for fixing that.

Regards,
Hartley

Re: [PATCH] mtd: Fix kernel NULL pointer dereference in physmap.c

[Atsushi Nemoto]

On Tue, 20 Oct 2009 18:28:59 -0400, "H Hartley Sweeten" <hartleys@visionengravers.com> wrote:
> > In an attempt to improve my responsiveness as maintainer, I'd already
> > committed the first version. How does this look:
> 
> Very responsive indeed.  ;-)
> 
> Sorry about introducing the bug.  Your amended patch looks like it serves
> the same purpose as my updated one.  Thanks for fixing that.

Thank you.  Both looks good for me.

Reviewed-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>