All of lore.kernel.org
 help / color / mirror / Atom feed
From: Schspa Shi <schspa@gmail.com>
To: Luis Chamberlain <mcgrof@kernel.org>
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: Thu, 15 Dec 2022 13:11:39 +0800	[thread overview]
Message-ID: <m24jtx9pbp.fsf@gmail.com> (raw)
In-Reply-To: <Y5orL90JpH6LUd4D@bombadil.infradead.org>


Luis Chamberlain <mcgrof@kernel.org> writes:

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

I think a syntactic sugar for a complete api can be added here for a
generic usage.

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

Ok, let's talk about this on the v1 patch's thread.

>   Luis

-- 
BRs
Schspa Shi

      reply	other threads:[~2022-12-15  6: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
2022-12-15  5:11   ` Schspa Shi [this message]

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=m24jtx9pbp.fsf@gmail.com \
    --to=schspa@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --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.