[PATCH] mtd: parsers: qcom: Fix leaking of partition name

[PATCH] mtd: parsers: qcom: Fix leaking of partition name

[Ansuel Smith]

Partition name are already allocated and defined by the ptable struct.
Skip reallocation of name variable and directly lowercase the name in
the ptable struct. The name variable was duplicated but never freed.
The leak was found using kmemleak.

Fixes: 803eb124e1a6 ("mtd: parsers: Add Qcom SMEM parser")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/mtd/parsers/qcomsmempart.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/parsers/qcomsmempart.c b/drivers/mtd/parsers/qcomsmempart.c
index d9083308f6ba..3d083f0815f8 100644
--- a/drivers/mtd/parsers/qcomsmempart.c
+++ b/drivers/mtd/parsers/qcomsmempart.c
@@ -62,8 +62,8 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
 	struct smem_flash_ptable *ptable;
 	size_t len = SMEM_FLASH_PTABLE_HDR_LEN;
 	struct mtd_partition *parts;
-	int ret, i, numparts;
-	char *name, *c;
+	int i, numparts;
+	char *c;
 
 	if (IS_ENABLED(CONFIG_MTD_SPI_NOR_USE_4K_SECTORS)
 			&& mtd->type == MTD_NORFLASH) {
@@ -125,17 +125,11 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
 		if (pentry->name[0] == '\0')
 			continue;
 
-		name = kstrdup(pentry->name, GFP_KERNEL);
-		if (!name) {
-			ret = -ENOMEM;
-			goto out_free_parts;
-		}
-
 		/* Convert name to lower case */
-		for (c = name; *c != '\0'; c++)
+		for (c = pentry->name; *c != '\0'; c++)
 			*c = tolower(*c);
 
-		parts[i].name = name;
+		parts[i].name = pentry->name;
 		parts[i].offset = le32_to_cpu(pentry->offset) * mtd->erasesize;
 		parts[i].mask_flags = pentry->attr;
 		parts[i].size = le32_to_cpu(pentry->length) * mtd->erasesize;
@@ -149,14 +143,6 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
 	*pparts = parts;
 
 	return numparts;
-
-out_free_parts:
-	while (--i >= 0)
-		kfree(parts[i].name);
-	kfree(parts);
-	*pparts = NULL;
-
-	return ret;
 }
 
 static const struct of_device_id qcomsmem_of_match_table[] = {
-- 
2.31.1

Re: [PATCH] mtd: parsers: qcom: Fix leaking of partition name

[Bjorn Andersson]

On Mon 24 May 19:14 CDT 2021, Ansuel Smith wrote:

> Partition name are already allocated and defined by the ptable struct.
> Skip reallocation of name variable and directly lowercase the name in
> the ptable struct. The name variable was duplicated but never freed.
> The leak was found using kmemleak.
> 

We know that SMEM_AARM_PARTITION_TABLE is accessed by at least the modem
firmware, so I don't think we should change it in place.

I think the appropriate solution is to implement the "cleanup" operation
in struct mtd_part_parser.

Regards,
Bjorn

> Fixes: 803eb124e1a6 ("mtd: parsers: Add Qcom SMEM parser")
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/mtd/parsers/qcomsmempart.c | 22 ++++------------------
>  1 file changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/parsers/qcomsmempart.c b/drivers/mtd/parsers/qcomsmempart.c
> index d9083308f6ba..3d083f0815f8 100644
> --- a/drivers/mtd/parsers/qcomsmempart.c
> +++ b/drivers/mtd/parsers/qcomsmempart.c
> @@ -62,8 +62,8 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
>  	struct smem_flash_ptable *ptable;
>  	size_t len = SMEM_FLASH_PTABLE_HDR_LEN;
>  	struct mtd_partition *parts;
> -	int ret, i, numparts;
> -	char *name, *c;
> +	int i, numparts;
> +	char *c;
>  
>  	if (IS_ENABLED(CONFIG_MTD_SPI_NOR_USE_4K_SECTORS)
>  			&& mtd->type == MTD_NORFLASH) {
> @@ -125,17 +125,11 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
>  		if (pentry->name[0] == '\0')
>  			continue;
>  
> -		name = kstrdup(pentry->name, GFP_KERNEL);
> -		if (!name) {
> -			ret = -ENOMEM;
> -			goto out_free_parts;
> -		}
> -
>  		/* Convert name to lower case */
> -		for (c = name; *c != '\0'; c++)
> +		for (c = pentry->name; *c != '\0'; c++)
>  			*c = tolower(*c);
>  
> -		parts[i].name = name;
> +		parts[i].name = pentry->name;
>  		parts[i].offset = le32_to_cpu(pentry->offset) * mtd->erasesize;
>  		parts[i].mask_flags = pentry->attr;
>  		parts[i].size = le32_to_cpu(pentry->length) * mtd->erasesize;
> @@ -149,14 +143,6 @@ static int parse_qcomsmem_part(struct mtd_info *mtd,
>  	*pparts = parts;
>  
>  	return numparts;
> -
> -out_free_parts:
> -	while (--i >= 0)
> -		kfree(parts[i].name);
> -	kfree(parts);
> -	*pparts = NULL;
> -
> -	return ret;
>  }
>  
>  static const struct of_device_id qcomsmem_of_match_table[] = {
> -- 
> 2.31.1
>