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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx>
> > Analyzed-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > Cc: Kay Sievers <kay@xxxxxxxx>
> > Signed-off-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> > ---
> >  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