[PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Tetsuo Handa]

syzbot is hitting use-after-free bug in uinput module [1]. This is because
kobject_uevent(KOBJ_REMOVE) is called again due to commit 0f4dafc0563c6c49
("Kobject: auto-cleanup on final unref") after memory allocation fault
injection made kobject_uevent(KOBJ_REMOVE) from device_del() from
input_unregister_device() fail, while uinput_destroy_device() is expecting
that kobject_uevent(KOBJ_REMOVE) is not called after device_del() from
input_unregister_device() completed. Fix this problem by marking "remove"
event done regardless of result.

[1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d

Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
Analyzed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Kay Sievers <kay@vrfy.org>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 lib/kobject_uevent.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index f058026..7ec4165 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -466,6 +466,13 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 	int i = 0;
 	int retval = 0;
 
+	/*
+	 * Mark "remove" event done regardless of result, for some subsystems
+	 * do not want to re-trigger "remove" event via automatic cleanup.
+	 */
+	if (action == KOBJ_REMOVE && kobj->state_add_uevent_sent)
+		kobj->state_remove_uevent_sent = 1;
+
 	pr_debug("kobject: '%s' (%p): %s\n",
 		 kobject_name(kobj), kobj, __func__);
 
@@ -567,10 +574,6 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
 		kobj->state_add_uevent_sent = 1;
 		break;
 
-	case KOBJ_REMOVE:
-		kobj->state_remove_uevent_sent = 1;
-		break;
-
 	case KOBJ_UNBIND:
 		zap_modalias_env(env);
 		break;
-- 
1.8.3.1

Re: [PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Greg Kroah-Hartman]

On Wed, Feb 20, 2019 at 10:38:34PM +0900, Tetsuo Handa wrote:
> syzbot is hitting use-after-free bug in uinput module [1]. This is because
> kobject_uevent(KOBJ_REMOVE) is called again due to commit 0f4dafc0563c6c49
> ("Kobject: auto-cleanup on final unref") after memory allocation fault
> injection made kobject_uevent(KOBJ_REMOVE) from device_del() from
> input_unregister_device() fail, while uinput_destroy_device() is expecting
> that kobject_uevent(KOBJ_REMOVE) is not called after device_del() from
> input_unregister_device() completed. Fix this problem by marking "remove"
> event done regardless of result.
> 
> [1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d
> 
> Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
> Analyzed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: Kay Sievers <kay@vrfy.org>
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  lib/kobject_uevent.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> index f058026..7ec4165 100644
> --- a/lib/kobject_uevent.c
> +++ b/lib/kobject_uevent.c
> @@ -466,6 +466,13 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
>  	int i = 0;
>  	int retval = 0;
>  
> +	/*
> +	 * Mark "remove" event done regardless of result, for some subsystems
> +	 * do not want to re-trigger "remove" event via automatic cleanup.
> +	 */
> +	if (action == KOBJ_REMOVE && kobj->state_add_uevent_sent)
> +		kobj->state_remove_uevent_sent = 1;
> +
>  	pr_debug("kobject: '%s' (%p): %s\n",
>  		 kobject_name(kobj), kobj, __func__);

If you really want to do this, put it below the debugging line.

But I would argue that this is not ok, as the remove uevent did NOT get
sent, and you are saying it did.

What memory is being used-after-free here when we fail to properly send
a uevent?  Shouldn't we fix up that problem correctly?

thanks,

greg k-h

Re: [PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Dmitry Torokhov]

On Wed, Feb 20, 2019 at 7:07 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Feb 20, 2019 at 10:38:34PM +0900, Tetsuo Handa wrote:
> > syzbot is hitting use-after-free bug in uinput module [1]. This is because
> > kobject_uevent(KOBJ_REMOVE) is called again due to commit 0f4dafc0563c6c49
> > ("Kobject: auto-cleanup on final unref") after memory allocation fault
> > injection made kobject_uevent(KOBJ_REMOVE) from device_del() from
> > input_unregister_device() fail, while uinput_destroy_device() is expecting
> > that kobject_uevent(KOBJ_REMOVE) is not called after device_del() from
> > input_unregister_device() completed. Fix this problem by marking "remove"
> > event done regardless of result.
> >
> > [1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d
> >
> > Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
> > Analyzed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Cc: Kay Sievers <kay@vrfy.org>
> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> > ---
> >  lib/kobject_uevent.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
> > index f058026..7ec4165 100644
> > --- a/lib/kobject_uevent.c
> > +++ b/lib/kobject_uevent.c
> > @@ -466,6 +466,13 @@ int kobject_uevent_env(struct kobject *kobj, enum kobject_action action,
> >       int i = 0;
> >       int retval = 0;
> >
> > +     /*
> > +      * Mark "remove" event done regardless of result, for some subsystems
> > +      * do not want to re-trigger "remove" event via automatic cleanup.
> > +      */
> > +     if (action == KOBJ_REMOVE && kobj->state_add_uevent_sent)
> > +             kobj->state_remove_uevent_sent = 1;
> > +
> >       pr_debug("kobject: '%s' (%p): %s\n",
> >                kobject_name(kobj), kobj, __func__);
>
> If you really want to do this, put it below the debugging line.
>
> But I would argue that this is not ok, as the remove uevent did NOT get
> sent, and you are saying it did.

"It is the thought that counts" here. The code was added to catch
cases where nobody even attempted to send "remove" uevents. It does
not guarantee that an event will ultimately be sent (we are at the
point of no return as far as the rest of the kernel is concerned,
there are no repeats or do-overs).

>
> What memory is being used-after-free here when we fail to properly send
> a uevent?  Shouldn't we fix up that problem correctly?

This is the correct fix (in spirit, we may argue about whether it is
valid to call the flag "state_add_uevent_sent" now or we should or
collapse both it and "state_add_uevent_sent" into
"need_send_remove_uevent"). Other subsystems are in their own right to
not expect to get uvent callbacks past the point of calling
device_del() as they are tearing down parts of the device environment
(even though the device structure may stay in memory for a while).

Thanks.

-- 
Dmitry

Re: [PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Tetsuo Handa]

On 2019/02/21 4:52, Dmitry Torokhov wrote:
> On Wed, Feb 20, 2019 at 7:07 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>> But I would argue that this is not ok, as the remove uevent did NOT get
>> sent, and you are saying it did.
> 
> "It is the thought that counts" here. The code was added to catch
> cases where nobody even attempted to send "remove" uevents. It does
> not guarantee that an event will ultimately be sent (we are at the
> point of no return as far as the rest of the kernel is concerned,
> there are no repeats or do-overs).
> 
>>
>> What memory is being used-after-free here when we fail to properly send
>> a uevent?  Shouldn't we fix up that problem correctly?

It is explained at https://lkml.kernel.org/r/20190219185558.GA210481@dtor-ws .

> 
> This is the correct fix (in spirit, we may argue about whether it is
> valid to call the flag "state_add_uevent_sent" now or we should or
> collapse both it and "state_add_uevent_sent" into
> "need_send_remove_uevent"). Other subsystems are in their own right to
> not expect to get uvent callbacks past the point of calling
> device_del() as they are tearing down parts of the device environment
> (even though the device structure may stay in memory for a while).
> 
> Thanks.

Which subsystems benefit from commit 0f4dafc0563c6c49 ("Kobject: auto-cleanup
on final unref") ? If there is no such subsystem, it will be better to remove
state_add_uevent_sent and state_remove_uevent_sent logic.

Re: [PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Greg Kroah-Hartman]

On Thu, Feb 21, 2019 at 07:40:20PM +0900, Tetsuo Handa wrote:
> On 2019/02/21 4:52, Dmitry Torokhov wrote:
> > On Wed, Feb 20, 2019 at 7:07 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> >> But I would argue that this is not ok, as the remove uevent did NOT get
> >> sent, and you are saying it did.
> > 
> > "It is the thought that counts" here. The code was added to catch
> > cases where nobody even attempted to send "remove" uevents. It does
> > not guarantee that an event will ultimately be sent (we are at the
> > point of no return as far as the rest of the kernel is concerned,
> > there are no repeats or do-overs).
> > 
> >>
> >> What memory is being used-after-free here when we fail to properly send
> >> a uevent?  Shouldn't we fix up that problem correctly?
> 
> It is explained at https://lkml.kernel.org/r/20190219185558.GA210481@dtor-ws .

I agree with Dmitry here, it's not the input layer's job to do this.

But that didn't answer my question, what object is getting reused here?
The input device is now dead at that point in time, no one else can
touch it, but you are saying someone did, right?   Who is that someone?

> > This is the correct fix (in spirit, we may argue about whether it is
> > valid to call the flag "state_add_uevent_sent" now or we should or
> > collapse both it and "state_add_uevent_sent" into
> > "need_send_remove_uevent"). Other subsystems are in their own right to
> > not expect to get uvent callbacks past the point of calling
> > device_del() as they are tearing down parts of the device environment
> > (even though the device structure may stay in memory for a while).
> > 
> > Thanks.
> 
> Which subsystems benefit from commit 0f4dafc0563c6c49 ("Kobject: auto-cleanup
> on final unref") ? If there is no such subsystem, it will be better to remove
> state_add_uevent_sent and state_remove_uevent_sent logic.

You are asking me about a patch from 2007.  I have no idea, try removing
it and see what happens :)

greg k-h

Re: [PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Tetsuo Handa]

On 2019/02/21 20:09, Greg Kroah-Hartman wrote:
> On Thu, Feb 21, 2019 at 07:40:20PM +0900, Tetsuo Handa wrote:
>> On 2019/02/21 4:52, Dmitry Torokhov wrote:
>>> On Wed, Feb 20, 2019 at 7:07 AM Greg Kroah-Hartman
>>> <gregkh@linuxfoundation.org> wrote:
>>>> But I would argue that this is not ok, as the remove uevent did NOT get
>>>> sent, and you are saying it did.
>>>
>>> "It is the thought that counts" here. The code was added to catch
>>> cases where nobody even attempted to send "remove" uevents. It does
>>> not guarantee that an event will ultimately be sent (we are at the
>>> point of no return as far as the rest of the kernel is concerned,
>>> there are no repeats or do-overs).
>>>
>>>>
>>>> What memory is being used-after-free here when we fail to properly send
>>>> a uevent?  Shouldn't we fix up that problem correctly?
>>
>> It is explained at https://lkml.kernel.org/r/20190219185558.GA210481@dtor-ws .
> 
> I agree with Dmitry here, it's not the input layer's job to do this.
> 
> But that didn't answer my question, what object is getting reused here?

"struct input_dev"->{name,phys} fields are accessed after they are kfree()d,
for uinput_destroy_device() sometimes kfree()s dev->{name,phys} at
uinput_destroy_device() before dev_uevent() is triggered by dropping the
refcount to 0. syzbot is hitting this bug from cdev_put() path when closing
a character file which drops the refcount to 0.

Therefore, I thought that we must not assume that kobject_uevent() won't be
called after uinput_destroy_device() called kfree(). And I wrote a patch
that explicitly sets "struct input_dev"->{name,phys} to NULL so that
input_dev_uevent() won't crash, for the timing of triggering last
input_put_device() is uncontrollable. But Dmitry Torokhov pointed out that
we should fix kobject side because input subsystem is not expecting that
kobject_uevent() is called after uinput_destroy_device().

> The input device is now dead at that point in time, no one else can
> touch it, but you are saying someone did, right?   Who is that someone?
> 
>>> This is the correct fix (in spirit, we may argue about whether it is
>>> valid to call the flag "state_add_uevent_sent" now or we should or
>>> collapse both it and "state_add_uevent_sent" into
>>> "need_send_remove_uevent"). Other subsystems are in their own right to
>>> not expect to get uvent callbacks past the point of calling
>>> device_del() as they are tearing down parts of the device environment
>>> (even though the device structure may stay in memory for a while).
>>>
>>> Thanks.
>>
>> Which subsystems benefit from commit 0f4dafc0563c6c49 ("Kobject: auto-cleanup
>> on final unref") ? If there is no such subsystem, it will be better to remove
>> state_add_uevent_sent and state_remove_uevent_sent logic.
> 
> You are asking me about a patch from 2007.  I have no idea, try removing
> it and see what happens :)

Waiting for response from Kay Sievers who wrote that patch. ;-)

Re: [PATCH] kobject: Don't trigger kobject_uevent(KOBJ_REMOVE) twice.

[Tetsuo Handa]

On 2019/02/21 21:31, Tetsuo Handa wrote:
> On 2019/02/21 20:09, Greg Kroah-Hartman wrote:
>>>> This is the correct fix (in spirit, we may argue about whether it is
>>>> valid to call the flag "state_add_uevent_sent" now or we should or
>>>> collapse both it and "state_add_uevent_sent" into
>>>> "need_send_remove_uevent"). Other subsystems are in their own right to
>>>> not expect to get uvent callbacks past the point of calling
>>>> device_del() as they are tearing down parts of the device environment
>>>> (even though the device structure may stay in memory for a while).
>>>>
>>>> Thanks.
>>>
>>> Which subsystems benefit from commit 0f4dafc0563c6c49 ("Kobject: auto-cleanup
>>> on final unref") ? If there is no such subsystem, it will be better to remove
>>> state_add_uevent_sent and state_remove_uevent_sent logic.
>>
>> You are asking me about a patch from 2007.  I have no idea, try removing
>> it and see what happens :)
> 
> Waiting for response from Kay Sievers who wrote that patch. ;-)
> 

No response from Kay Sievers...

Well, can we apply this patch for now because making sure that
"state_remove_uevent_sent won't be attempted twice" should be safer for
stable kernels than "removing the state_{add,remove}_uevent_sent logic" ?