[PATCH] init/initramfs.c: check the return value of kstrdup()

[PATCH] init/initramfs.c: check the return value of kstrdup()

[xkernel.wang]

From: Xiaoke Wang <xkernel.wang@foxmail.com>

kstrdup() is also a memory allocation function which is similar
with kmalloc() in some way. Once some internal memory errors
happen, it will return NULL. It is better to check the return
value of it so to catch the memory error in time.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 init/initramfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index a842c05..49deffb 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -139,8 +139,12 @@ static void __init dir_add(const char *name, time64_t mtime)
 	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
 	if (!de)
 		panic_show_mem("can't allocate dir_entry buffer");
-	INIT_LIST_HEAD(&de->list);
 	de->name = kstrdup(name, GFP_KERNEL);
+	if (!de->name) {
+		kfree(de);
+		panic_show_mem("can't duplicate dir name");
+	}
+	INIT_LIST_HEAD(&de->list);
 	de->mtime = mtime;
 	list_add(&de->list, &dir_list);
 }
-- 

Re: [PATCH] init/initramfs.c: check the return value of kstrdup()

[Greg KH]

On Fri, Mar 04, 2022 at 05:27:34PM +0800, xkernel.wang@foxmail.com wrote:
> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> kstrdup() is also a memory allocation function which is similar
> with kmalloc() in some way. Once some internal memory errors
> happen, it will return NULL. It is better to check the return
> value of it so to catch the memory error in time.
> 
> Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
> ---
>  init/initramfs.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/init/initramfs.c b/init/initramfs.c
> index a842c05..49deffb 100644
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -139,8 +139,12 @@ static void __init dir_add(const char *name, time64_t mtime)
>  	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
>  	if (!de)
>  		panic_show_mem("can't allocate dir_entry buffer");
> -	INIT_LIST_HEAD(&de->list);
>  	de->name = kstrdup(name, GFP_KERNEL);
> +	if (!de->name) {

How can this fail?  Have you ever hit this in real life?

> +		kfree(de);
> +		panic_show_mem("can't duplicate dir name");

Why are you freeing memory if you are panicing?

How was this tested?

thanks,

greg k-h

Re: [PATCH] init/initramfs.c: check the return value of kstrdup()

[Xiaoke Wang]

On Fri, 04 Mar 2022 22:14:21 +0800, Greg KH <gregkh@linuxfoundation.org> wrote:
>> struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
>> if (!de)
>>  panic_show_mem("can't allocate dir_entry buffer");
>> - INIT_LIST_HEAD(&de->list);
>> de->name = kstrdup(name, GFP_KERNEL);
>> + if (!de->name) {
>
> How can this fail?  Have you ever hit this in real life?
>
>> +kfree(de);
>> +panic_show_mem("can't duplicate dir name");
>
> Why are you freeing memory if you are panicing?
>
> How was this tested?

Thank you for taking the time.
I found this with a static tool, without dynamic testing.
kstrdup() allocates memory for copying the string and I noticed all the
other allocation functions in this file have the check for their return
value such as `de` on the above code. So I suppose this is also needed
to be checked and I intuitively add kfree() on the error path.
I'm sorry to bother you if this is actually unnecessary.

Regards,
Xiaoke Wang

Re: [PATCH] init/initramfs.c: check the return value of kstrdup()

[Andrew Morton]

On Fri,  4 Mar 2022 17:27:34 +0800 xkernel.wang@foxmail.com wrote:

> From: Xiaoke Wang <xkernel.wang@foxmail.com>
> 
> kstrdup() is also a memory allocation function which is similar
> with kmalloc() in some way. Once some internal memory errors
> happen, it will return NULL. It is better to check the return
> value of it so to catch the memory error in time.
> 
> ...
>
> --- a/init/initramfs.c
> +++ b/init/initramfs.c
> @@ -139,8 +139,12 @@ static void __init dir_add(const char *name, time64_t mtime)
>  	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
>  	if (!de)
>  		panic_show_mem("can't allocate dir_entry buffer");
> -	INIT_LIST_HEAD(&de->list);
>  	de->name = kstrdup(name, GFP_KERNEL);
> +	if (!de->name) {
> +		kfree(de);
> +		panic_show_mem("can't duplicate dir name");
> +	}
> +	INIT_LIST_HEAD(&de->list);
>  	de->mtime = mtime;
>  	list_add(&de->list, &dir_list);

We often assume that memory allocations cannot fail in __init code.  If
the kernel runs out of memory at this stage, we have very deep problems
and it's virtually impossible that execution would have got this far.

[PATCH] init/initramfs.c: check the return value of kstrdup()

[Xiaoke Wang]

kstrdup() is also a memory allocation function and it is similar
with kmalloc() in some way. Once some internal memory errors
happen, it will return NULL. It is better to check the return
value of it so to catch the memory error in time.

Signed-off-by: Xiaoke Wang <xkernel.wang@foxmail.com>
---
 init/initramfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/init/initramfs.c b/init/initramfs.c
index a842c05..49deffb 100644
--- a/init/initramfs.c
+++ b/init/initramfs.c
@@ -139,8 +139,12 @@ static void __init dir_add(const char *name, time64_t mtime)
 	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
 	if (!de)
 		panic_show_mem("can't allocate dir_entry buffer");
-	INIT_LIST_HEAD(&de->list);
 	de->name = kstrdup(name, GFP_KERNEL);
+	if (!de->name) {
+		kfree(de);
+		panic_show_mem("can't duplicate dir name");
+	}
+	INIT_LIST_HEAD(&de->list);
 	de->mtime = mtime;
 	list_add(&de->list, &dir_list);
 }
--