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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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