[PATCH] kthread: potential dereference of null pointer

[PATCH] kthread: potential dereference of null pointer

[Jiasheng Jiang]

The return value of kzalloc() needs to be checked.
To avoid use of null pointer in case of the failure of alloc.

Fixes: dc6a87f5450d ("sched: Make the idle task quack like a per-CPU kthread")
Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
---
 kernel/kthread.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 08931e525dd9..3feefeff4922 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -101,6 +101,8 @@ void set_kthread_struct(struct task_struct *p)
 		return;
 
 	kthread = kzalloc(sizeof(*kthread), GFP_KERNEL);
+	if (!kthread)
+		return;
 	/*
 	 * We abuse ->set_child_tid to avoid the new member and because it
 	 * can't be wrongly copied by copy_process(). We also rely on fact
-- 
2.25.1

Re: [PATCH] kthread: potential dereference of null pointer

[Petr Mladek]

On Thu 2021-12-09 14:43:14, Jiasheng Jiang wrote:
> The return value of kzalloc() needs to be checked.
> To avoid use of null pointer in case of the failure of alloc.
> 
> Fixes: dc6a87f5450d ("sched: Make the idle task quack like a per-CPU
> kthread")

The hash id looks wrong:

$> git log -p -1 dc6a87f5450d
fatal: ambiguous argument 'dc6a87f5450d': unknown revision or path not in the working tree.


> Signed-off-by: Jiasheng Jiang <jiasheng@iscas.ac.cn>
> ---
>  kernel/kthread.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 08931e525dd9..3feefeff4922 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -101,6 +101,8 @@ void set_kthread_struct(struct task_struct *p)
>  		return;
>  
>  	kthread = kzalloc(sizeof(*kthread), GFP_KERNEL);
> +	if (!kthread)
> +		return;

This does not have any effect. It will only skip the assignment:

	p->set_child_tid = (__force void __user *)kthread;

But we are here only when p->set_child_tid is already NULL
because of the above check:

	if (__to_kthread(p))
		return;


>  	/*
>  	 * We abuse ->set_child_tid to avoid the new member and because it
>  	 * can't be wrongly copied by copy_process(). We also rely on fact

By other words. The change does not harm but it is not needed either.
Anyway, the commit message is misleading. It suggests that it fixes
something but it is not true.

I would personally keep the code as is. The original code makes it
more clear that the allocation failure is not handled.

Best Regards,
Petr

Re: [PATCH] kthread: potential dereference of null pointer

[Andrew Morton]

On Thu,  9 Dec 2021 14:43:14 +0800 Jiasheng Jiang <jiasheng@iscas.ac.cn> wrote:

> The return value of kzalloc() needs to be checked.
> To avoid use of null pointer in case of the failure of alloc.
> 
> ...
>
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -101,6 +101,8 @@ void set_kthread_struct(struct task_struct *p)
>  		return;
>  
>  	kthread = kzalloc(sizeof(*kthread), GFP_KERNEL);
> +	if (!kthread)
> +		return;
>  	/*
>  	 * We abuse ->set_child_tid to avoid the new member and because it
>  	 * can't be wrongly copied by copy_process(). We also rely on fact

No, we shouldn't simply leave ->set_child_tid uninitialized if kmalloc
failed.

set_ktread_struct() appears to be designed so that callers must check
that to_kthread() returns non-zero after having called
set_kthread_struct().

Which is a quite weird interface, but I'm not seeing any bugs here.  If
kthread() sees to_kthread()==NULL then this kthread won't be created
and all the other unchecked calls to to_kthread() will never execute,
because this kthread doesn't exist.

The exception is in init_idle(), but that's __init code, executed at
boot time when we assume that allocations will always succeed.