From patchwork Tue Apr 26 00:54:18 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tetsuo Handa X-Patchwork-Id: 12826398 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2A9D2C433F5 for ; Tue, 26 Apr 2022 00:54:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234974AbiDZA55 (ORCPT ); Mon, 25 Apr 2022 20:57:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35644 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230268AbiDZA54 (ORCPT ); Mon, 25 Apr 2022 20:57:56 -0400 Received: from www262.sakura.ne.jp (www262.sakura.ne.jp [202.181.97.72]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1CD2A11F61F for ; Mon, 25 Apr 2022 17:54:51 -0700 (PDT) Received: from fsav311.sakura.ne.jp (fsav311.sakura.ne.jp [153.120.85.142]) by www262.sakura.ne.jp (8.15.2/8.15.2) with ESMTP id 23Q0sIQj032078; Tue, 26 Apr 2022 09:54:18 +0900 (JST) (envelope-from penguin-kernel@I-love.SAKURA.ne.jp) Received: from www262.sakura.ne.jp (202.181.97.72) by fsav311.sakura.ne.jp (F-Secure/fsigk_smtp/550/fsav311.sakura.ne.jp); Tue, 26 Apr 2022 09:54:18 +0900 (JST) X-Virus-Status: clean(F-Secure/fsigk_smtp/550/fsav311.sakura.ne.jp) Received: from [192.168.1.9] (M106072142033.v4.enabler.ne.jp [106.72.142.33]) (authenticated bits=0) by www262.sakura.ne.jp (8.15.2/8.15.2) with ESMTPSA id 23Q0sI7Z032075 (version=TLSv1.2 cipher=AES256-GCM-SHA384 bits=256 verify=NO); Tue, 26 Apr 2022 09:54:18 +0900 (JST) (envelope-from penguin-kernel@I-love.SAKURA.ne.jp) Message-ID: <349f3e34-41ed-f832-3b22-ae10c50e3868@I-love.SAKURA.ne.jp> Date: Tue, 26 Apr 2022 09:54:18 +0900 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: [PATCH] media: imon: remove redundant serialization Content-Language: en-US To: Sean Young , Alan Stern Cc: Jarod Wilson , Mauro Carvalho Chehab , syzbot , andreyknvl@google.com, linux-media@vger.kernel.org, linux-usb@vger.kernel.org, syzkaller-bugs@googlegroups.com References: <00000000000043b599058faf0145@google.com> <5a06c7f1-9a29-99e4-c700-fec3f09509d2@I-love.SAKURA.ne.jp> <62dddbb9-3053-f284-f9db-3beda5e8e951@I-love.SAKURA.ne.jp> From: Tetsuo Handa In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org Since usb_register_dev() from imon_init_display() from imon_probe() holds minor_rwsem while display_open() which holds driver_lock and ictx->lock is called with minor_rwsem held from usb_open(), holding driver_lock or ictx->lock when calling usb_register_dev() causes circular locking dependency problem. Since usb_deregister_dev() from imon_disconnect() holds minor_rwsem while display_open() which holds driver_lock is called with minor_rwsem held, holding driver_lock when calling usb_deregister_dev() also causes circular locking dependency problem. Sean Young explained that the problem is there are imon devices which have two usb interfaces, even though it is one device. The probe and disconnect function of both usb interfaces can run concurrently. Alan Stern responded that the driver and USB cores guarantee that when an interface is probed, both the interface and its USB device are locked. Ditto for when the disconnect callback gets run. So concurrent probing/ disconnection of multiple interfaces on the same device is not possible. Therefore, simply remove redundant serialization. Link: https://syzkaller.appspot.com/bug?extid=c558267ad910fc494497 Reported-by: syzbot Signed-off-by: Tetsuo Handa Cc: Sean Young Cc: Alan Stern --- This patch might be OK for solving circular locking dependency problem. But https://syzkaller.appspot.com/text?tag=CrashReport&x=1632f89f700000 says free_imon_context() from imon_disconnect() can call kfree(ictx) before display_close() is called, resulting in use-after-free problem. I guess that we need to defer free_imon_context() using refcount till display_close() is called. Fix as a separate patch or include into this patch? drivers/media/rc/imon.c | 21 --------------------- 1 file changed, 21 deletions(-) diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c index 54da6f60079b..e0e893d96cf3 100644 --- a/drivers/media/rc/imon.c +++ b/drivers/media/rc/imon.c @@ -439,9 +439,6 @@ static struct usb_driver imon_driver = { .id_table = imon_usb_id_table, }; -/* to prevent races between open() and disconnect(), probing, etc */ -static DEFINE_MUTEX(driver_lock); - /* Module bookkeeping bits */ MODULE_AUTHOR(MOD_AUTHOR); MODULE_DESCRIPTION(MOD_DESC); @@ -499,9 +496,6 @@ static int display_open(struct inode *inode, struct file *file) int subminor; int retval = 0; - /* prevent races with disconnect */ - mutex_lock(&driver_lock); - subminor = iminor(inode); interface = usb_find_interface(&imon_driver, subminor); if (!interface) { @@ -534,7 +528,6 @@ static int display_open(struct inode *inode, struct file *file) mutex_unlock(&ictx->lock); exit: - mutex_unlock(&driver_lock); return retval; } @@ -2416,9 +2409,6 @@ static int imon_probe(struct usb_interface *interface, dev_dbg(dev, "%s: found iMON device (%04x:%04x, intf%d)\n", __func__, vendor, product, ifnum); - /* prevent races probing devices w/multiple interfaces */ - mutex_lock(&driver_lock); - first_if = usb_ifnum_to_if(usbdev, 0); if (!first_if) { ret = -ENODEV; @@ -2456,8 +2446,6 @@ static int imon_probe(struct usb_interface *interface, usb_set_intfdata(interface, ictx); if (ifnum == 0) { - mutex_lock(&ictx->lock); - if (product == 0xffdc && ictx->rf_device) { sysfs_err = sysfs_create_group(&interface->dev.kobj, &imon_rf_attr_group); @@ -2468,21 +2456,17 @@ static int imon_probe(struct usb_interface *interface, if (ictx->display_supported) imon_init_display(ictx, interface); - - mutex_unlock(&ictx->lock); } dev_info(dev, "iMON device (%04x:%04x, intf%d) on usb<%d:%d> initialized\n", vendor, product, ifnum, usbdev->bus->busnum, usbdev->devnum); - mutex_unlock(&driver_lock); usb_put_dev(usbdev); return 0; fail: - mutex_unlock(&driver_lock); usb_put_dev(usbdev); dev_err(dev, "unable to register, err %d\n", ret); @@ -2498,9 +2482,6 @@ static void imon_disconnect(struct usb_interface *interface) struct device *dev; int ifnum; - /* prevent races with multi-interface device probing and display_open */ - mutex_lock(&driver_lock); - ictx = usb_get_intfdata(interface); dev = ictx->dev; ifnum = interface->cur_altsetting->desc.bInterfaceNumber; @@ -2545,8 +2526,6 @@ static void imon_disconnect(struct usb_interface *interface) if (!ictx->dev_present_intf0 && !ictx->dev_present_intf1) free_imon_context(ictx); - mutex_unlock(&driver_lock); - dev_dbg(dev, "%s: iMON device (intf%d) disconnected\n", __func__, ifnum); }