| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <m2ili43s2v.fsf@gmail.com>
Date: Thu, 22 Dec 2022 13:45:46 +0800
From: Schspa Shi <schspa@...il.com>
To: Luis Chamberlain <mcgrof@...nel.org>
Cc: mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com,
vincent.guittot@...aro.org, dietmar.eggemann@....com,
rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
bristot@...hat.com, vschneid@...hat.com,
linux-kernel@...r.kernel.org,
syzbot+10d19d528d9755d9af22@...kaller.appspotmail.com,
syzbot+70d5d5d83d03db2c813d@...kaller.appspotmail.com,
syzbot+83cb0411d0fcf0a30fc1@...kaller.appspotmail.com
Subject: Re: [PATCH] umh: fix UAF when the process is being killed
Schspa Shi <schspa@...il.com> writes:
> Luis Chamberlain <mcgrof@...nel.org> writes:
>
>> Peter, Ingo, Steven would like you're review.
>>
>> On Tue, Dec 13, 2022 at 03:03:53PM -0800, Luis Chamberlain wrote:
>>> On Mon, Dec 12, 2022 at 09:38:31PM +0800, Schspa Shi wrote:
>>> > I'd like to upload a V2 patch with the new solution if you prefer the
>>> > following way.
>>> >
>>> > diff --git a/kernel/umh.c b/kernel/umh.c
>>> > index 850631518665..8023f11fcfc0 100644
>>> > --- a/kernel/umh.c
>>> > +++ b/kernel/umh.c
>>> > @@ -452,6 +452,11 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
>>> > /* umh_complete() will see NULL and free sub_info */
>>> > if (xchg(&sub_info->complete, NULL))
>>> > goto unlock;
>>> > + /*
>>> > + * kthreadd (or new kernel thread) will call complete()
>>> > + * shortly.
>>> > + */
>>> > + wait_for_completion(&done);
>>> > }
>>>
>>> Yes much better. Did you verify it fixes the splat found by the bots?
>>
>> Wait, I'm not sure yet why this would fix it... I first started thinking
>> that this may be a good example of a Coccinelle SmPL rule, something like:
>>
>> DECLARE_COMPLETION_ONSTACK(done);
>> foo *foo;
>> ...
>> foo->completion = &done;
>> ...
>> queue_work(system_unbound_wq, &foo->work);
>> ....
>> ret = wait_for_completion_state(&done, state);
>> ...
>> if (!ret)
>> S
>> ...
>> +wait_for_completion(&done);
>>
>> But that is pretty complex, and while it may be useful to know how many
>> patterns we have like this, it begs the question if generalizing this
>> inside the callers is best for -ERESTARTSYS condition is best. What
>> do folks think?
>>
>> The rationale here is that if you queue stuff and give access to the
>> completion variable but its on-stack obviously you can end up with the
>> queued stuff complete() on a on-stack variable. The issue seems to
>> be that wait_for_completion_state() for -ERESTARTSYS still means
>> that the already scheduled queue'd work is *about* to run and
>> the process with the completion on-stack completed. So we race with
>> the end of the routine and the completion on-stack.
>>
>> It makes me wonder if wait_for_completion() above really is doing
>> something more, if it is just helping with timing and is still error
>> prone.
>>
>> The queued work will try the the completion as follows:
>>
>> static void umh_complete(struct subprocess_info *sub_info)
>> {
>> struct completion *comp = xchg(&sub_info->complete, NULL);
>> /*
>> * See call_usermodehelper_exec(). If xchg() returns NULL
>> * we own sub_info, the UMH_KILLABLE caller has gone away
>> * or the caller used UMH_NO_WAIT.
>> */
>> if (comp)
>> complete(comp);
>> else
>> call_usermodehelper_freeinfo(sub_info);
>> }
>>
>> So the race is getting -ERESTARTSYS on the process with completion
>> on-stack and the above running complete(comp). Why would sprinkling
>> wait_for_completion(&done) *after* wait_for_completion_state(&done, state)
>> fix this UAF?
>
> The wait_for_completion(&done) is added when xchg(&sub_info->complete,
> NULL) return NULL. When it returns NULL, it means the umh_complete was
> using the completion variable at the same time and will call complete
> in a very short time.
>
Hi Luis:
Is there any further progress on this problem? Does the above
explanation answer your doubts?
> Add wait_for_completion *after* wait_for_completion_state will make the
> interruptible/timeout version API not working anymore.
>
>> }
>> diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
>> index d57a5c1c1cd9..aa7031faca04 100644
>> --- a/kernel/sched/completion.c
>> +++ b/kernel/sched/completion.c
>> @@ -205,8 +205,10 @@ int __sched wait_for_completion_interruptible(struct completion *x)
>> {
>> long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE);
>>
>> - if (t == -ERESTARTSYS)
>> + if (t == -ERESTARTSYS) {
>> + wait_for_completion(x);
>> return t;
>> + }
>> return 0;
>> }
>> EXPORT_SYMBOL(wait_for_completion_interruptible);
>> @@ -243,8 +245,10 @@ int __sched wait_for_completion_killable(struct completion *x)
>> {
>> long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_KILLABLE);
>>
>> - if (t == -ERESTARTSYS)
>> + if (t == -ERESTARTSYS) {
>> + wait_for_completion(x);
>> return t;
>> + }
>> return 0;
>> }
>> EXPORT_SYMBOL(wait_for_completion_killable);
>> @@ -253,8 +257,10 @@ int __sched wait_for_completion_state(struct completion *x, unsigned int state)
>> {
>> long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, state);
>>
>> - if (t == -ERESTARTSYS)
>> + if (t == -ERESTARTSYS) {
>> + wait_for_completion(x);
>> return t;
>> + }
>> return 0;
>> }
>> EXPORT_SYMBOL(wait_for_completion_state);
>
> If we want to make it a generic fix, syntactic sugar can be added to
> simplify usage for users.
>
> Consider the following patch.
>
> diff --git a/kernel/sched/completion.c b/kernel/sched/completion.c
> index d57a5c1c1cd9..67b7d02c0098 100644
> --- a/kernel/sched/completion.c
> +++ b/kernel/sched/completion.c
> @@ -341,3 +341,33 @@ bool completion_done(struct completion *x)
> return true;
> }
> EXPORT_SYMBOL(completion_done);
> +
> +void complete_on_stack(struct completion **x)
> +{
> + struct completion *comp = xchg(*x, NULL);
> +
> + if (comp)
> + complete(comp);
> +}
> +EXPORT_SYMBOL(complete_on_stack);
> +
> +int __sched wait_for_completion_state_on_stack(struct completion **x,
> + unsigned int state)
> +{
> + struct completion *comp = *x;
> + int retval;
> +
> + retval = wait_for_completion_state(comp, state);
> + if (retval) {
> + if (xchg(*x, NULL))
> + return retval;
> +
> + /*
> + * complete_on_stack will call complete shortly.
> + */
> + wait_for_completion(comp);
> + }
> +
> + return retval;
> +}
> +EXPORT_SYMBOL(wait_for_completion_state_on_stack);
--
BRs
Schspa Shi
Powered by blists - more mailing lists