Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Feb 2022 16:23:46 -0500
From:   Alan Stern <stern@rowland.harvard.edu>
To:     syzbot <syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com>
Cc:     gregkh@linuxfoundation.org, heikki.krogerus@linux.intel.com,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
        linux-input@vger.kernel.org, noralf@tronnes.org,
        syzkaller-bugs@googlegroups.com, tzimmermann@suse.de
Subject: Re: [syzbot] memory leak in hub_event (3)
Message-ID: <YgbT4uqSIVY9ku10@rowland.harvard.edu>
References: <0000000000005cacef05d7c3c10d@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <0000000000005cacef05d7c3c10d@google.com>
Precedence: bulk

On Fri, Feb 11, 2022 at 12:17:26PM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    dfd42facf1e4 Linux 5.17-rc3
> git tree:       upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=14b4ef7c700000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=48b71604a367da6e
> dashboard link: https://syzkaller.appspot.com/bug?extid=8caaaec4e7a55d75e243
> compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=1396902c700000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=1466e662700000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+8caaaec4e7a55d75e243@syzkaller.appspotmail.com
> 
> BUG: memory leak
> unreferenced object 0xffff88810d49e800 (size 2048):
>   comm "kworker/1:1", pid 25, jiffies 4294954629 (age 16.460s)
>   hex dump (first 32 bytes):
>     ff ff ff ff 31 00 00 00 00 00 00 00 00 00 00 00  ....1...........
>     00 00 00 00 00 00 00 00 00 00 00 00 03 00 00 00  ................
>   backtrace:
>     [<ffffffff82c87a62>] kmalloc include/linux/slab.h:581 [inline]
>     [<ffffffff82c87a62>] kzalloc include/linux/slab.h:715 [inline]
>     [<ffffffff82c87a62>] usb_alloc_dev+0x32/0x450 drivers/usb/core/usb.c:582
>     [<ffffffff82c91a47>] hub_port_connect drivers/usb/core/hub.c:5260 [inline]
>     [<ffffffff82c91a47>] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline]
>     [<ffffffff82c91a47>] port_event drivers/usb/core/hub.c:5660 [inline]
>     [<ffffffff82c91a47>] hub_event+0x1097/0x21a0 drivers/usb/core/hub.c:5742
>     [<ffffffff8126c3ef>] process_one_work+0x2bf/0x600 kernel/workqueue.c:2307
>     [<ffffffff8126ccd9>] worker_thread+0x59/0x5b0 kernel/workqueue.c:2454
>     [<ffffffff81276765>] kthread+0x125/0x160 kernel/kthread.c:377
>     [<ffffffff810022ff>] ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295

There's a refcount leak in the probe-failure path of the hid-elo driver.  
(You can see that this is the relevant driver in the console output.)  
It doesn't need the refcount anyway, because the elo_priv structure is 
always deallocated synchronously before the elo_remove routine returns.

(Syzbot isn't always all that great at deducing where the real problem 
lies when something goes wrong.)

Alan Stern

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/ v5.17-rc3

Index: usb-devel/drivers/hid/hid-elo.c
===================================================================
--- usb-devel.orig/drivers/hid/hid-elo.c
+++ usb-devel/drivers/hid/hid-elo.c
@@ -239,7 +239,7 @@ static int elo_probe(struct hid_device *
 
 	INIT_DELAYED_WORK(&priv->work, elo_work);
 	udev = interface_to_usbdev(to_usb_interface(hdev->dev.parent));
-	priv->usbdev = usb_get_dev(udev);
+	priv->usbdev = udev;
 
 	hid_set_drvdata(hdev, priv);
 
@@ -270,8 +270,6 @@ static void elo_remove(struct hid_device
 {
 	struct elo_priv *priv = hid_get_drvdata(hdev);
 
-	usb_put_dev(priv->usbdev);
-
 	hid_hw_stop(hdev);
 	cancel_delayed_work_sync(&priv->work);
 	kfree(priv);