diff mbox

mtd: Fix kernel NULL pointer dereference in physmap.c

Message ID BD79186B4FD85F4B8E60E381CAEE190901E246A0@mi8nycmail19.Mi8.com
State Accepted
Commit 4b56ffcacee937a85bf39e14872dd141e23ee85f
Headers show

Commit Message

Hartley Sweeten Oct. 19, 2009, 5:31 p.m. UTC 
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>

---

Comments

Atsushi Nemoto Oct. 20, 2009, 3:29 p.m. UTC | #1 
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
Hartley Sweeten Oct. 20, 2009, 4:08 p.m. UTC | #2 
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
Atsushi Nemoto Oct. 20, 2009, 4:17 p.m. UTC | #3 
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
Hartley Sweeten Oct. 20, 2009, 4:52 p.m. UTC | #4 
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
diff mbox

Patch

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;