Patchwork mtd: Fix kernel NULL pointer dereference in physmap.c

login
register
mail settings
Submitter hartleys
Date Oct. 19, 2009, 5:31 p.m.
Message ID <BD79186B4FD85F4B8E60E381CAEE190901E246A0@mi8nycmail19.Mi8.com>
Download mbox | patch
Permalink /patch/36389/
State Accepted
Commit 4b56ffcacee937a85bf39e14872dd141e23ee85f
Headers show

Comments

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

---
Atsushi Nemoto - Oct. 20, 2009, 3:29 p.m.
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
hartleys - Oct. 20, 2009, 4:08 p.m.
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.
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
hartleys - Oct. 20, 2009, 4:52 p.m.
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

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;