[PATCH] pstore/ram: Fix memory leak in persistent_ram_new

[PATCH] pstore/ram: Fix memory leak in persistent_ram_new

[Navid Emamdoost]

In the implementation of persistent_ram_new(), if the allocation for prz
fails, "label" should be released as part of error handling. Release the
label via kfree().

Fixes: 8cf5aff89e59 ("staging: android: persistent_ram: Introduce persistent_ram_new()")
Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>
---
 fs/pstore/ram_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
index 8823f65888f0..7d2d86999211 100644
--- a/fs/pstore/ram_core.c
+++ b/fs/pstore/ram_core.c
@@ -568,6 +568,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
 	prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
 	if (!prz) {
 		pr_err("failed to allocate persistent ram zone\n");
+		kfree(label);
 		goto err;
 	}
 
-- 
2.17.1

Re: [PATCH] pstore/ram: Fix memory leak in persistent_ram_new

[Markus Elfring]

> In the implementation of persistent_ram_new(), if the allocation for prz
> fails, "label" should be released as part of error handling.

How did you come to this conclusion?

I suggest to reconsider this patch because the mentioned string is passed by
a function parameter.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/fs/pstore/ram_core.c?id=37d4e84f765bb3038ddfeebdc5d1cfd7e1ef688f#n561
https://elixir.bootlin.com/linux/v5.5-rc1/source/fs/pstore/ram_core.c#L561

I recognise other implementation details which could be improved here.
https://lore.kernel.org/lkml/97737d95-d6aa-d24f-1af0-9d4895ceaed2@users.sourceforge.net/
https://lkml.org/lkml/2017/8/16/645

Regards,
Markus

Re: [PATCH] pstore/ram: Fix memory leak in persistent_ram_new

[Kees Cook]

On Wed, Dec 11, 2019 at 01:13:51PM -0600, Navid Emamdoost wrote:
> In the implementation of persistent_ram_new(), if the allocation for prz
> fails, "label" should be released as part of error handling. Release the
> label via kfree().
> 
> Fixes: 8cf5aff89e59 ("staging: android: persistent_ram: Introduce persistent_ram_new()")
> Signed-off-by: Navid Emamdoost <navid.emamdoost@gmail.com>

You're right about the need to clean up the allocation, but I think it's
on the caller to do this, not persistent_ram_new(), in case a const char
string is used in the future.

I think the "Fixes" should also be:
Fixes: 1227daa43bce ("pstore/ram: Clarify resource reservation labels")

-Kees

> ---
>  fs/pstore/ram_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c
> index 8823f65888f0..7d2d86999211 100644
> --- a/fs/pstore/ram_core.c
> +++ b/fs/pstore/ram_core.c
> @@ -568,6 +568,7 @@ struct persistent_ram_zone *persistent_ram_new(phys_addr_t start, size_t size,
>  	prz = kzalloc(sizeof(struct persistent_ram_zone), GFP_KERNEL);
>  	if (!prz) {
>  		pr_err("failed to allocate persistent ram zone\n");
> +		kfree(label);
>  		goto err;
>  	}
>  
> -- 
> 2.17.1
> 

-- 
Kees Cook