Re: [PATCH] umh: fix UAF when the process is being killed

[Schspa Shi <schspa@gmail.com>]

Schspa Shi <schspa@gmail.com> writes:

> When the process is killed, wait_for_completion_state will return with
> -ERESTARTSYS, and the completion variable in the stack will be freed.
> If the user-mode thread is complete at the same time, there will be a UAF.
>
> Please refer to the following scenarios.
>             T1                                  T2
> ------------------------------------------------------------------
> call_usermodehelper_exec
>                                    call_usermodehelper_exec_async
>                                    << do something >>
>                                    umh_complete(sub_info);
>                                    comp = xchg(&sub_info->complete, NULL);
>                                    /* we got the completion */
>                                    << context switch >>
>
>     << Being killed >>
> 	retval = wait_for_completion_state(sub_info->complete, state);
> 	if (!retval)
> 		goto wait_done;
>
> 	if (wait & UMH_KILLABLE) {
> 		/* umh_complete() will see NULL and free sub_info */
> 		if (xchg(&sub_info->complete, NULL))
> 			goto unlock;
>         << we can't got the completion >>
> 	}
> 	....
> unlock:
> 	helper_unlock();
> 	return retval;
> }
>
> /**
>  * the completion variable in stack is end of life cycle.
>  * and maybe freed due to process is recycled.
>  */
>                                    --------UAF here----------
>                                    if (comp)
>                                        complete(comp);
>
> To fix it, we can put the completion variable in the subprocess_info
> variable.
>
> Reported-by: syzbot+10d19d528d9755d9af22@syzkaller.appspotmail.com
> Reported-by: syzbot+70d5d5d83d03db2c813d@syzkaller.appspotmail.com
> Reported-by: syzbot+83cb0411d0fcf0a30fc1@syzkaller.appspotmail.com
>
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---
>  include/linux/umh.h | 1 +
>  kernel/umh.c        | 6 +++---
>  2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/umh.h b/include/linux/umh.h
> index 5d1f6129b847..801f7efbc825 100644
> --- a/include/linux/umh.h
> +++ b/include/linux/umh.h
> @@ -20,6 +20,7 @@ struct file;
>  struct subprocess_info {
>  	struct work_struct work;
>  	struct completion *complete;
> +	struct completion done;
>  	const char *path;
>  	char **argv;
>  	char **envp;
> diff --git a/kernel/umh.c b/kernel/umh.c
> index 850631518665..3ed39956c777 100644
> --- a/kernel/umh.c
> +++ b/kernel/umh.c
> @@ -380,6 +380,7 @@ struct subprocess_info *call_usermodehelper_setup(const char *path, char **argv,
>  	sub_info->cleanup = cleanup;
>  	sub_info->init = init;
>  	sub_info->data = data;
> +	init_completion(&sub_info->done);
>    out:
>  	return sub_info;
>  }
> @@ -405,7 +406,6 @@ EXPORT_SYMBOL(call_usermodehelper_setup);
>  int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  {
>  	unsigned int state = TASK_UNINTERRUPTIBLE;
> -	DECLARE_COMPLETION_ONSTACK(done);
>  	int retval = 0;
>  
>  	if (!sub_info->path) {
> @@ -431,7 +431,7 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  	 * This makes it possible to use umh_complete to free
>  	 * the data structure in case of UMH_NO_WAIT.
>  	 */
> -	sub_info->complete = (wait == UMH_NO_WAIT) ? NULL : &done;
> +	sub_info->complete = (wait == UMH_NO_WAIT) ? NULL : &sub_info->done;
>  	sub_info->wait = wait;
>  
>  	queue_work(system_unbound_wq, &sub_info->work);
> @@ -444,7 +444,7 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>  	if (wait & UMH_FREEZABLE)
>  		state |= TASK_FREEZABLE;
>  
> -	retval = wait_for_completion_state(&done, state);
> +	retval = wait_for_completion_state(sub_info->complete, state);
>  	if (!retval)
>  		goto wait_done;

Hi Luis Chamberlain:

Could you help to review this patch? I'm not sure why we define the
amount of completion here on the stack. But this UAF can be fixed by
moving the completion variable to the heap.

-- 
BRs
Schspa Shi