All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mtd: parsers: qcom: Fix leaking of partition name
@ 2021-05-25  0:14 ` Ansuel Smith
  0 siblings, 0 replies; 4+ messages in thread
From: Ansuel Smith @ 2021-05-25  0:14 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Manivannan Sadhasivam, linux-arm-msm,
	linux-mtd, linux-kernel
  Cc: 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


^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH] mtd: parsers: qcom: Fix leaking of partition name
@ 2021-05-25  0:14 ` Ansuel Smith
  0 siblings, 0 replies; 4+ messages in thread
From: Ansuel Smith @ 2021-05-25  0:14 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Manivannan Sadhasivam, linux-arm-msm,
	linux-mtd, linux-kernel
  Cc: 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


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] mtd: parsers: qcom: Fix leaking of partition name
  2021-05-25  0:14 ` Ansuel Smith
@ 2021-05-25  3:38   ` Bjorn Andersson
  -1 siblings, 0 replies; 4+ messages in thread
From: Bjorn Andersson @ 2021-05-25  3:38 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andy Gross, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Manivannan Sadhasivam, linux-arm-msm,
	linux-mtd, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] mtd: parsers: qcom: Fix leaking of partition name
@ 2021-05-25  3:38   ` Bjorn Andersson
  0 siblings, 0 replies; 4+ messages in thread
From: Bjorn Andersson @ 2021-05-25  3:38 UTC (permalink / raw)
  To: Ansuel Smith
  Cc: Andy Gross, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Manivannan Sadhasivam, linux-arm-msm,
	linux-mtd, linux-kernel

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
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-05-25  3:56 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-25  0:14 [PATCH] mtd: parsers: qcom: Fix leaking of partition name Ansuel Smith
2021-05-25  0:14 ` Ansuel Smith
2021-05-25  3:38 ` Bjorn Andersson
2021-05-25  3:38   ` Bjorn Andersson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.