All of lore.kernel.org
 help / color / mirror / Atom feed
From: Luis Chamberlain <mcgrof@kernel.org>
To: Schspa Shi <schspa@gmail.com>
Cc: linux-kernel@vger.kernel.org,
	syzbot+10d19d528d9755d9af22@syzkaller.appspotmail.com,
	syzbot+70d5d5d83d03db2c813d@syzkaller.appspotmail.com,
	syzbot+83cb0411d0fcf0a30fc1@syzkaller.appspotmail.com,
	syzbot+c92c6a251d49ceceb625@syzkaller.appspotmail.com
Subject: Re: [PATCH v2] umh: fix out of scope usage when the process is being killed
Date: Wed, 14 Dec 2022 11:59:43 -0800	[thread overview]
Message-ID: <Y5orL90JpH6LUd4D@bombadil.infradead.org> (raw)
In-Reply-To: <20221214134656.21228-1-schspa@gmail.com>

On Wed, Dec 14, 2022 at 09:46:56PM +0800, Schspa Shi wrote:
> When the process is killed, wait_for_completion_state will return with
> -ERESTARTSYS, and the completion variable in the stack will be unavailable,
> even freed. If the user-mode thread is complete at the same time, there
> will be a race to use a unavailable variable.
> 
> 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, because T2 take it already >>
> 	}
> 	....
> 	return retval;
> }
> 
> /**
>  * the completion variable in stack is end of life cycle.
>  * and maybe freed due to process is recycled.
>  */
>                                    -------- BUG here----------
>                                    if (comp)
>                                        complete(comp);
> 
> To fix it, we can add an additional wait_for_completion to ensure the
> completion object is completely unused. And this is what
> kthread_create_on_node does to handle this race.
> 
> Reported-by: syzbot+10d19d528d9755d9af22@syzkaller.appspotmail.com
> Reported-by: syzbot+70d5d5d83d03db2c813d@syzkaller.appspotmail.com
> Reported-by: syzbot+83cb0411d0fcf0a30fc1@syzkaller.appspotmail.com
> Reported-by: syzbot+c92c6a251d49ceceb625@syzkaller.appspotmail.com
> Signed-off-by: Schspa Shi <schspa@gmail.com>
> ---

Please fix the commit log a bit more with the cotext I provided, *if*
on the other thread the community agrees with the approach to be
compartamentalized. I am still not sure why this would fix the
UAF after thinking about it some more, and the issue would mean
there likely could be a generic fix / issue to consider.

So for now I'd like more review of this race and the proposed fix
as I mentioned in the follow-up threaad in your v1 patch. Let's
follow up there and see how that discussion goes.

  Luis

  reply	other threads:[~2022-12-14 20:06 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-14 13:46 [PATCH v2] umh: fix out of scope usage when the process is being killed Schspa Shi
2022-12-14 19:59 ` Luis Chamberlain [this message]
2022-12-15  5:11   ` Schspa Shi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y5orL90JpH6LUd4D@bombadil.infradead.org \
    --to=mcgrof@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=schspa@gmail.com \
    --cc=syzbot+10d19d528d9755d9af22@syzkaller.appspotmail.com \
    --cc=syzbot+70d5d5d83d03db2c813d@syzkaller.appspotmail.com \
    --cc=syzbot+83cb0411d0fcf0a30fc1@syzkaller.appspotmail.com \
    --cc=syzbot+c92c6a251d49ceceb625@syzkaller.appspotmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.