[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.

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2972 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <tencent_B8A4989923779B6381A23A5C4209FD7F1E05@qq.com>
References: <tencent_B8A4989923779B6381A23A5C4209FD7F1E05@qq.com>
TO: Xiaoke Wang <xkernel.wang@foxmail.com>
TO: linux-kernel(a)vger.kernel.org
CC: Xiaoke Wang <xkernel.wang@foxmail.com>

Hi Xiaoke,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.16-rc5]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Xiaoke-Wang/init-initramfs-c-check-the-return-value-of-kstrdup/20211213-170026
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 136057256686de39cc3a07c2e39ef6bc43003ff6
:::::: branch date: 12 hours ago
:::::: commit date: 12 hours ago
config: i386-randconfig-m021-20211213 (https://download.01.org/0day-ci/archive/20211214/202112140423.37WdeaEE-lkp(a)intel.com/config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

New smatch warnings:
init/initramfs.c:148 dir_add() error: dereferencing freed memory 'de'

Old smatch warnings:
init/initramfs.c:97 find_link() error: we previously assumed 'q' could be null (see line 95)
init/initramfs.c:142 dir_add() error: we previously assumed 'de' could be null (see line 140)

vim +/de +148 init/initramfs.c

889d51a10712b6 Nye Liu          2008-10-15  136  
e35c4c64fe492b Arnd Bergmann    2017-11-17  137  static void __init dir_add(const char *name, time64_t mtime)
889d51a10712b6 Nye Liu          2008-10-15  138  {
889d51a10712b6 Nye Liu          2008-10-15  139  	struct dir_entry *de = kmalloc(sizeof(struct dir_entry), GFP_KERNEL);
889d51a10712b6 Nye Liu          2008-10-15  140  	if (!de)
dd23e8098f33a5 Florian Fainelli 2021-02-25  141  		panic_show_mem("can't allocate dir_entry buffer");
889d51a10712b6 Nye Liu          2008-10-15  142  	de->name = kstrdup(name, GFP_KERNEL);
764baf9ece4791 Xiaoke Wang      2021-12-13  143  	if (!de->name) {
764baf9ece4791 Xiaoke Wang      2021-12-13  144  		kfree(de);
764baf9ece4791 Xiaoke Wang      2021-12-13  145  		panic_show_mem("can't duplicate dir name");
764baf9ece4791 Xiaoke Wang      2021-12-13  146  	}
764baf9ece4791 Xiaoke Wang      2021-12-13  147  	INIT_LIST_HEAD(&de->list);
889d51a10712b6 Nye Liu          2008-10-15 @148  	de->mtime = mtime;
889d51a10712b6 Nye Liu          2008-10-15  149  	list_add(&de->list, &dir_list);
889d51a10712b6 Nye Liu          2008-10-15  150  }
889d51a10712b6 Nye Liu          2008-10-15  151  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[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);
 }
--