[PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Atsushi Nemoto]

The mtd partition parser returns an allocated pointer array of
mtd_partition.  The caller must free it.  The array is used only for
add_mtd_partitions(), so free it just after the call.

Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
---
 drivers/mtd/maps/physmap.c |   17 ++++++++---------
 1 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index 7ca048d..cc26b41 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -29,7 +29,6 @@ struct physmap_flash_info {
 	struct map_info		map[MAX_RESOURCES];
 #ifdef CONFIG_MTD_PARTITIONS
 	int			nr_parts;
-	struct mtd_partition	*parts;
 #endif
 };
 
@@ -56,14 +55,10 @@ static int physmap_flash_remove(struct platform_device *dev)
 	for (i = 0; i < MAX_RESOURCES; i++) {
 		if (info->mtd[i] != NULL) {
 #ifdef CONFIG_MTD_PARTITIONS
-			if (info->nr_parts) {
+			if (info->nr_parts || physmap_data->nr_parts)
 				del_mtd_partitions(info->mtd[i]);
-				kfree(info->parts);
-			} else if (physmap_data->nr_parts) {
-				del_mtd_partitions(info->mtd[i]);
-			} else {
+			else
 				del_mtd_device(info->mtd[i]);
-			}
 #else
 			del_mtd_device(info->mtd[i]);
 #endif
@@ -86,6 +81,9 @@ static int physmap_flash_probe(struct platform_device *dev)
 	int err = 0;
 	int i;
 	int devices_found = 0;
+#ifdef CONFIG_MTD_PARTITIONS
+	struct mtd_partition *parts;
+#endif
 
 	physmap_data = dev->dev.platform_data;
 	if (physmap_data == NULL)
@@ -166,9 +164,10 @@ static int physmap_flash_probe(struct platform_device *dev)
 		goto err_out;
 
 #ifdef CONFIG_MTD_PARTITIONS
-	err = parse_mtd_partitions(info->cmtd, part_probe_types, &info->parts, 0);
+	err = parse_mtd_partitions(info->cmtd, part_probe_types, &parts, 0);
 	if (err > 0) {
-		add_mtd_partitions(info->cmtd, info->parts, err);
+		add_mtd_partitions(info->cmtd, parts, err);
+		kfree(parts);
 		return 0;
 	}
 
-- 
1.5.6.3

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Atsushi Nemoto]

On Wed, 12 Nov 2008 23:57:33 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> The mtd partition parser returns an allocated pointer array of
> mtd_partition.  The caller must free it.  The array is used only for
> add_mtd_partitions(), so free it just after the call.

Note: all parsers except for the ar7part return kmalloced memory.

The ar7part parser returns a pointer of static array.  While currently
there is no in-kernel user of this parser, there should not be cause
regression.

Anyway, I suppose ar7part parser also should return kmalloced memory,
as like as all other parsers.

---
Atsushi Nemoto

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Atsushi Nemoto]

On Wed, 12 Nov 2008 23:57:33 +0900 (JST), Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:
> The mtd partition parser returns an allocated pointer array of
> mtd_partition.  The caller must free it.  The array is used only for
> add_mtd_partitions(), so free it just after the call.
> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
>  drivers/mtd/maps/physmap.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)

And many other callers of parse_mtd_partitions() also do not free the
returned memory.  With a quick look, 21 of 36 have same defect.

drivers/mtd/devices/m25p80.c
drivers/mtd/devices/mtd_dataflash.c
drivers/mtd/maps/bfin-async-flash.c
drivers/mtd/maps/edb7312.c
drivers/mtd/maps/impa7.c
drivers/mtd/maps/intel_vr_nor.c
drivers/mtd/maps/solutionengine.c
drivers/mtd/nand/atmel_nand.c
drivers/mtd/nand/cafe_nand.c
drivers/mtd/nand/cmx270_nand.c
drivers/mtd/nand/cs553x_nand.c
drivers/mtd/nand/edb7312.c
drivers/mtd/nand/fsl_elbc_nand.c
drivers/mtd/nand/fsl_upm.c
drivers/mtd/nand/mxc_nand.c
drivers/mtd/nand/orion_nand.c
drivers/mtd/nand/ppchameleonevb.c
drivers/mtd/nand/sharpsl.c
drivers/mtd/nand/tmio_nand.c
drivers/mtd/nand/ts7250.c
drivers/mtd/onenand/generic.c

Volunteers? ;)

---
Atsushi Nemoto

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Mike Frysinger]

On Wed, Nov 12, 2008 at 10:55, Atsushi Nemoto wrote:
> On Wed, 12 Nov 2008 23:57:33 +0900 (JST), Atsushi Nemoto wrote:
>> The mtd partition parser returns an allocated pointer array of
>> mtd_partition.  The caller must free it.  The array is used only for
>> add_mtd_partitions(), so free it just after the call.
>>
>> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
>> ---
>>  drivers/mtd/maps/physmap.c |   17 ++++++++---------
>>  1 files changed, 8 insertions(+), 9 deletions(-)
>
> And many other callers of parse_mtd_partitions() also do not free the
> returned memory.  With a quick look, 21 of 36 have same defect.

i wonder why we duplicate this same code block in so many places.  and
why does every driver have to declare its own list of parsers ?  cant
we unify all of these in one place ?

> drivers/mtd/maps/bfin-async-flash.c

i can fix this one if no one else gets to it
-mikex

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[David Woodhouse]

On Wed, 2008-11-12 at 11:08 -0500, Mike Frysinger wrote:
> i wonder why we duplicate this same code block in so many places.  and
> why does every driver have to declare its own list of parsers ?  cant
> we unify all of these in one place ?

Well, most boards only really want _one_ partition type; maybe two. And
the probes can be quite expensive and have false positives, so we don't
want to be doing all the probes for all devices.

It might make sense to just pass a bitmask indicating which partition
probes to use. Assuming we can agree on an ordering, that is :)

Another complication is that some boards have registered the whole
device first, while others have just registered the partitions.

I suspect it might be better to take the simple bug-fix approach for
now, given that we are planning a revamp of the MTD core API anyway --
we can redo partitions properly at the point, so they aren't just an
evil hack.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Artem Bityutskiy]

On Tue, 2008-11-18 at 13:57 +0000, David Woodhouse wrote:
> I suspect it might be better to take the simple bug-fix approach for
> now, given that we are planning a revamp of the MTD core API anyway --
> we can redo partitions properly at the point, so they aren't just an
> evil hack.

Off topic: is this going to really happen? Are you have real plans
to do this?

Also, you also mentioned that you have sysfs for MTD, are we going to
see them in mtd-2.6.git any time soon?

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[David Woodhouse]

On Tue, 2008-11-18 at 16:04 +0200, Artem Bityutskiy wrote:
> Off topic: is this going to really happen? Are you have real plans
> to do this?

The plans for partitioning aren't entirely defined, but yes -- we have
to do it.

> Also, you also mentioned that you have sysfs for MTD, are we going to
> see them in mtd-2.6.git any time soon?

I did a bunch of it when I was on the plane to the kernel summit and
plumbers conference. I should get it to actually compile and then put it
in a separate tree -- I don't think it'll be ready for merging to Linus
imminently.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Sascha Hauer]

Hi,

Sorry to reply to such an old thread, but...

On Wed, Nov 12, 2008 at 11:57:33PM +0900, Atsushi Nemoto wrote:
> The mtd partition parser returns an allocated pointer array of
> mtd_partition.  The caller must free it.  The array is used only for
> add_mtd_partitions(), so free it just after the call.

This patch breaks command line parsing support. With command line
partition parsing the struct mtd_partition array is allocated, but only
once. On my board with NAND and NOR (both with command line partition
parsing) It fails badly in parse_cmdline_partitions() when the second
device gets parsed.

The following patch fixes it, but I don't know if this is
the correct solution. Does anybody have more insights on this?

Sascha

diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
index 50a3403..14c00dd 100644
--- a/drivers/mtd/cmdlinepart.c
+++ b/drivers/mtd/cmdlinepart.c
@@ -335,7 +335,13 @@ static int parse_cmdline_partitions(struct mtd_info *master,
 				}
 				offset += part->parts[i].size;
 			}
-			*pparts = part->parts;
+
+			*pparts = kmalloc(sizeof (struct mtd_partition) * part->num_parts, GFP_KERNEL);
+			if (!*pparts)
+				return -ENOMEM;
+
+			memcpy(*pparts, part->parts, sizeof (struct mtd_partition) * part->num_parts);
+
 			return part->num_parts;
 		}
 	}

> 
> Signed-off-by: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> ---
>  drivers/mtd/maps/physmap.c |   17 ++++++++---------
>  1 files changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
> index 7ca048d..cc26b41 100644
> --- a/drivers/mtd/maps/physmap.c
> +++ b/drivers/mtd/maps/physmap.c
> @@ -29,7 +29,6 @@ struct physmap_flash_info {
>  	struct map_info		map[MAX_RESOURCES];
>  #ifdef CONFIG_MTD_PARTITIONS
>  	int			nr_parts;
> -	struct mtd_partition	*parts;
>  #endif
>  };
>  
> @@ -56,14 +55,10 @@ static int physmap_flash_remove(struct platform_device *dev)
>  	for (i = 0; i < MAX_RESOURCES; i++) {
>  		if (info->mtd[i] != NULL) {
>  #ifdef CONFIG_MTD_PARTITIONS
> -			if (info->nr_parts) {
> +			if (info->nr_parts || physmap_data->nr_parts)
>  				del_mtd_partitions(info->mtd[i]);
> -				kfree(info->parts);
> -			} else if (physmap_data->nr_parts) {
> -				del_mtd_partitions(info->mtd[i]);
> -			} else {
> +			else
>  				del_mtd_device(info->mtd[i]);
> -			}
>  #else
>  			del_mtd_device(info->mtd[i]);
>  #endif
> @@ -86,6 +81,9 @@ static int physmap_flash_probe(struct platform_device *dev)
>  	int err = 0;
>  	int i;
>  	int devices_found = 0;
> +#ifdef CONFIG_MTD_PARTITIONS
> +	struct mtd_partition *parts;
> +#endif
>  
>  	physmap_data = dev->dev.platform_data;
>  	if (physmap_data == NULL)
> @@ -166,9 +164,10 @@ static int physmap_flash_probe(struct platform_device *dev)
>  		goto err_out;
>  
>  #ifdef CONFIG_MTD_PARTITIONS
> -	err = parse_mtd_partitions(info->cmtd, part_probe_types, &info->parts, 0);
> +	err = parse_mtd_partitions(info->cmtd, part_probe_types, &parts, 0);
>  	if (err > 0) {
> -		add_mtd_partitions(info->cmtd, info->parts, err);
> +		add_mtd_partitions(info->cmtd, parts, err);
> +		kfree(parts);
>  		return 0;
>  	}
>  
> -- 
> 1.5.6.3
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Atsushi Nemoto]

On Tue, 24 Feb 2009 14:35:05 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Nov 12, 2008 at 11:57:33PM +0900, Atsushi Nemoto wrote:
> > The mtd partition parser returns an allocated pointer array of
> > mtd_partition.  The caller must free it.  The array is used only for
> > add_mtd_partitions(), so free it just after the call.
> 
> This patch breaks command line parsing support. With command line
> partition parsing the struct mtd_partition array is allocated, but only
> once. On my board with NAND and NOR (both with command line partition
> parsing) It fails badly in parse_cmdline_partitions() when the second
> device gets parsed.
> 
> The following patch fixes it, but I don't know if this is
> the correct solution. Does anybody have more insights on this?

Do your NAND and NOR have same mtd-id?  The cmdlinepart allocates
mtd_partition aray for each mtd-id.  So usually another array will be
returned for NAND and NOR.

The physmap patch has another bug and fixes are on the way mainline:

http://git.infradead.org/mtd-2.6.git?a=commit;h=e480814f138cd5d78a8efe397756ba6b6518fdb6

But this seems not enough, as you wrote.  If multiple mtd have same
mtd-id, bad things can happen.  And more seriously, if I load physmap
driver _again_ after unload, cmdlinepart will return a freed pointer
on the second time.

Hmm, little memory leak is less serious than crash.  Now I start
thinking reverting the commit 176bf2e0 will be best for 2.6.29
release.

I'm not sure for long term solutions.

A) make all parsers return kmalloc-ed mtd_partition array each time
   and fix memory leak in each driver

B) make all parsers return mtd_partition array allocated only once,
   and fix drivers which free the mtd_partition array.

David, how do you think?

---
Atsushi Nemoto

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Sascha Hauer]

On Tue, Feb 24, 2009 at 11:36:00PM +0900, Atsushi Nemoto wrote:
> On Tue, 24 Feb 2009 14:35:05 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Wed, Nov 12, 2008 at 11:57:33PM +0900, Atsushi Nemoto wrote:
> > > The mtd partition parser returns an allocated pointer array of
> > > mtd_partition.  The caller must free it.  The array is used only for
> > > add_mtd_partitions(), so free it just after the call.
> > 
> > This patch breaks command line parsing support. With command line
> > partition parsing the struct mtd_partition array is allocated, but only
> > once. On my board with NAND and NOR (both with command line partition
> > parsing) It fails badly in parse_cmdline_partitions() when the second
> > device gets parsed.
> > 
> > The following patch fixes it, but I don't know if this is
> > the correct solution. Does anybody have more insights on this?
> 
> Do your NAND and NOR have same mtd-id?  The cmdlinepart allocates
> mtd_partition aray for each mtd-id.  So usually another array will be
> returned for NAND and NOR.

Yes, it allocates one array for NAND and one for NOR. The problem starts
in mtdpart_setup_real():

>		struct cmdline_mtd_partition *this_mtd;
> 		struct mtd_partition *parts;
> 
> 		[...]
> 
> 		/*
> 		 * parse one mtd. have it reserve memory for the
> 		 * struct cmdline_mtd_partition and the mtd-id string.
> 		 */
> 		parts = newpart(p + 1,		/* cmdline */
> 				&s,		/* out: updated cmdline ptr */
> 				&num_parts,	/* out: number of parts */
> 				0,		/* first partition */
> 				(unsigned char**)&this_mtd, /* out: extra mem */
> 				mtd_id_len + 1 + sizeof(*this_mtd) +
> 				sizeof(void*)-1 /*alignment*/);
>

Here we allocate the array containing not only the struct mtd_partition,
but also the names and the struct cmdline_mtd_partition...

> 	for(part = partitions; part; part = part->next)
> 	{
> 		if ((!mtd_id) || (!strcmp(part->mtd_id, mtd_id)))
> 		{
> 			for(i = 0, offset = 0; i < part->num_parts; i++)
> 			{
> 				if (part->parts[i].offset == OFFSET_CONTINUOUS)
> 				  part->parts[i].offset = offset;
> 				else
> 				  offset = part->parts[i].offset;
> 				if (part->parts[i].size == SIZE_REMAINING)
> 				  part->parts[i].size = master->size - offset;
> 				if (offset + part->parts[i].size > master->size)
> 				{
> 					printk(KERN_WARNING ERRP
> 					       "%s: partitioning exceeds flash size, truncating\n",
> 					       part->mtd_id);
> 					part->parts[i].size = master->size - offset;
> 					part->num_parts = i;
> 				}
> 				offset += part->parts[i].size;
> 			}
> 			*pparts = part->parts;

...here this array is returned and kfreed in the physmap flash driver. The
next time we enter this loop 'part' points to already freed memory.

*pparts is not meant to be freed, cmdlinepart.c needs it for the whole
kernel life. We can only return a copy of the partition array.

 
> 
> The physmap patch has another bug and fixes are on the way mainline:
> 
> http://git.infradead.org/mtd-2.6.git?a=commit;h=e480814f138cd5d78a8efe397756ba6b6518fdb6
> 
> But this seems not enough, as you wrote.  If multiple mtd have same
> mtd-id, bad things can happen.  And more seriously, if I load physmap
> driver _again_ after unload, cmdlinepart will return a freed pointer
> on the second time.
> 
> Hmm, little memory leak is less serious than crash.  Now I start
> thinking reverting the commit 176bf2e0 will be best for 2.6.29
> release.

Even when reverting the commit the same problem still exists because the
array then gets freed in physmap_flash_remove(). This won't hurt me
though because I never use mtd drivers as modules.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] physmap: Fix leak of memory returned by parse_mtd_partitions

[Atsushi Nemoto]

On Tue, 24 Feb 2009 16:29:58 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > The physmap patch has another bug and fixes are on the way mainline:
> > 
> > http://git.infradead.org/mtd-2.6.git?a=commit;h=e480814f138cd5d78a8efe397756ba6b6518fdb6
> > 
> > But this seems not enough, as you wrote.  If multiple mtd have same
> > mtd-id, bad things can happen.  And more seriously, if I load physmap
> > driver _again_ after unload, cmdlinepart will return a freed pointer
> > on the second time.
> > 
> > Hmm, little memory leak is less serious than crash.  Now I start
> > thinking reverting the commit 176bf2e0 will be best for 2.6.29
> > release.
> 
> Even when reverting the commit the same problem still exists because the
> array then gets freed in physmap_flash_remove(). This won't hurt me
> though because I never use mtd drivers as modules.

If the commit reverted, kfree() in physmap_flash_remove never be
called due to another bug (info->nr_parts is not set properly).  But
unloading the physmap module will lead crash anyway since master mtd
device will be freed without deleting slave mtd devices if cmdlinepart
was used.

So I think either reverting the commit or applying the above fix in
mtd-2.6 git tree can fix regression from 2.6.28.  Both work well
unless unloading the physmap module after booting with mtdparts=
option.

---
Atsushi Nemoto