Received: by 2002:a05:6358:bb9e:b0:b9:5105:a5b4 with SMTP id df30csp4424207rwb; Tue, 6 Sep 2022 07:22:00 -0700 (PDT) X-Google-Smtp-Source: AA6agR6Wd24Fo0Q1sFjhPdeXrag/oTUC8TBAyz+bF6iTW8fdg7qLYyPcf8xb0H5GAJgduL+s0U9Z X-Received: by 2002:a17:907:9493:b0:73b:e605:f3b with SMTP id dm19-20020a170907949300b0073be6050f3bmr41277263ejc.37.1662474120508; Tue, 06 Sep 2022 07:22:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1662474120; cv=none; d=google.com; s=arc-20160816; b=C1HmEtWF1F1mn9HR6R8nTjFLb97fFZGSt5KIs0DxCqsPDTq42Aee9OIQzSCraYiU2e dyW33n1Kupv72+XBJFhkovmdlvCjXB7KOPFY3EgHUMcwxt9MNGwLTCaDhH0/QlaBwnuj ZnHpMzTJk+5j0LxURcp1kkfIOc+I2vF/SaD2NH/QGc1hemdniqD/tFFgL2HdDUAENSZ6 N934aQgC3K5aBbffXHZxD820Eh+i2GAISQACi7h6gu0J21EDU+AUes5kJvJw1jrUYTE/ MZT8Jg+4vp63axMCPSyvaCjE3wm88oYYBWSkAvneePuypYMbB7UbAwIL0GizsCJGiD62 HC0A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:message-id:date:subject:cc:to :from:dkim-signature; bh=Ofl8x3I3mZpZN9aF84PpQ9LRRUjVQsrUI2A/9rp7vBY=; b=nReZs/QydQAdGM3BA+lTD7DTPHCIfXA1rN5cGomyPcRCwP0o2VfUDUIYDTcPg6LxY8 B/FJ4cxxItCsoRSXubxgRm6IgDHsz78SIi1U4QTxUitqTxlBm274ZbFDyEvxzd0ku2Fq 4c4JjJhL7ikaT3qi3h8KSqcmiRXTj/Ncq5ygiUd5oXF0YArhlOJeLd3Lu9CFM5k12Q34 G825/CkOjOqPeaYCLNit1AggI+tgj2o3Zc7zFZF0nhBg4h8i3pgS7+iJYQhJqZPuc+sE aoWnzuD9O/y8pLS9cGV8gLmRwbuX2Et6DH2ggzKdv1MUKvzJrfQGbmfqPDZ9BMXmQ1fc OVIQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=HXj3lg7V; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id ji2-20020a170907980200b0073d83260df4si10179570ejc.672.2022.09.06.07.21.26; Tue, 06 Sep 2022 07:22:00 -0700 (PDT) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@linuxfoundation.org header.s=korg header.b=HXj3lg7V; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linuxfoundation.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241011AbiIFOUT (ORCPT + 99 others); Tue, 6 Sep 2022 10:20:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35370 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241549AbiIFOR3 (ORCPT ); Tue, 6 Sep 2022 10:17:29 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id F3704186D7; Tue, 6 Sep 2022 06:49:44 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 37D1F6154D; Tue, 6 Sep 2022 13:48:45 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 444B8C433C1; Tue, 6 Sep 2022 13:48:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1662472124; bh=q6abylfIumjLHTob7RyN51s3fcoyH/H8kucmnlYEx+w=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=HXj3lg7VReyQ0fW41ToTOnV0iMGKoROxBmcx5wG1llZcwRB5HgE0pB6Ape+KnZk1F 814tt3sGoZ0D7V+2cxHQwtOjkOOvF1rDZk4ULifgTAil3YGIXJPZDKOMTiiawr4UKU 91GH1YV55m2sgnx1nNRMYH27ncdbZcBQMtF3DxDM= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Felipe Balbi , Marek Szyprowski , Alan Stern Subject: [PATCH 5.19 135/155] USB: gadget: Fix obscure lockdep violation for udc_mutex Date: Tue, 6 Sep 2022 15:31:23 +0200 Message-Id: <20220906132835.175305634@linuxfoundation.org> X-Mailer: git-send-email 2.37.3 In-Reply-To: <20220906132829.417117002@linuxfoundation.org> References: <20220906132829.417117002@linuxfoundation.org> User-Agent: quilt/0.67 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Alan Stern commit 1016fc0c096c92dd0e6e0541daac7a7868169903 upstream. A recent commit expanding the scope of the udc_lock mutex in the gadget core managed to cause an obscure and slightly bizarre lockdep violation. In abbreviated form: ====================================================== WARNING: possible circular locking dependency detected 5.19.0-rc7+ #12510 Not tainted ------------------------------------------------------ udevadm/312 is trying to acquire lock: ffff80000aae1058 (udc_lock){+.+.}-{3:3}, at: usb_udc_uevent+0x54/0xe0 but task is already holding lock: ffff000002277548 (kn->active#4){++++}-{0:0}, at: kernfs_seq_start+0x34/0xe0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (kn->active#4){++++}-{0:0}:        lock_acquire+0x68/0x84        __kernfs_remove+0x268/0x380        kernfs_remove_by_name_ns+0x58/0xac        sysfs_remove_file_ns+0x18/0x24        device_del+0x15c/0x440 -> #2 (device_links_lock){+.+.}-{3:3}:        lock_acquire+0x68/0x84        __mutex_lock+0x9c/0x430        mutex_lock_nested+0x38/0x64        device_link_remove+0x3c/0xa0        _regulator_put.part.0+0x168/0x190        regulator_put+0x3c/0x54        devm_regulator_release+0x14/0x20 -> #1 (regulator_list_mutex){+.+.}-{3:3}:        lock_acquire+0x68/0x84        __mutex_lock+0x9c/0x430        mutex_lock_nested+0x38/0x64        regulator_lock_dependent+0x54/0x284        regulator_enable+0x34/0x80        phy_power_on+0x24/0x130        __dwc2_lowlevel_hw_enable+0x100/0x130        dwc2_lowlevel_hw_enable+0x18/0x40        dwc2_hsotg_udc_start+0x6c/0x2f0        gadget_bind_driver+0x124/0x1f4 -> #0 (udc_lock){+.+.}-{3:3}:        __lock_acquire+0x1298/0x20cc        lock_acquire.part.0+0xe0/0x230        lock_acquire+0x68/0x84        __mutex_lock+0x9c/0x430        mutex_lock_nested+0x38/0x64        usb_udc_uevent+0x54/0xe0 Evidently this was caused by the scope of udc_mutex being too large. The mutex is only meant to protect udc->driver along with a few other things. As far as I can tell, there's no reason for the mutex to be held while the gadget core calls a gadget driver's ->bind or ->unbind routine, or while a UDC is being started or stopped. (This accounts for link #1 in the chain above, where the mutex is held while the dwc2_hsotg_udc is started as part of driver probing.) Gadget drivers' ->disconnect callbacks are problematic. Even though usb_gadget_disconnect() will now acquire the udc_mutex, there's a window in usb_gadget_bind_driver() between the times when the mutex is released and the ->bind callback is invoked. If a disconnect occurred during that window, we could call the driver's ->disconnect routine before its ->bind routine. To prevent this from happening, it will be necessary to prevent a UDC from connecting while it has no gadget driver. This should be done already but it doesn't seem to be; currently usb_gadget_connect() has no check for this. Such a check will have to be added later. Some degree of mutual exclusion is required in soft_connect_store(), which can dereference udc->driver at arbitrary times since it is a sysfs callback. The solution here is to acquire the gadget's device lock rather than the udc_mutex. Since the driver core guarantees that the device lock is always held during driver binding and unbinding, this will make the accesses in soft_connect_store() mutually exclusive with any changes to udc->driver. Lastly, it turns out there is one place which should hold the udc_mutex but currently does not: The function_show() routine needs protection while it dereferences udc->driver. The missing lock and unlock calls are added. Link: https://lore.kernel.org/all/b2ba4245-9917-e399-94c8-03a383e7070e@samsung.com/ Fixes: 2191c00855b0 ("USB: gadget: Fix use-after-free Read in usb_udc_uevent()") Cc: Felipe Balbi Cc: stable@vger.kernel.org Reported-by: Marek Szyprowski Tested-by: Marek Szyprowski Signed-off-by: Alan Stern Link: https://lore.kernel.org/r/YwkfhdxA/I2nOcK7@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman --- drivers/usb/gadget/udc/core.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad ret = gadget->ops->pullup(gadget, 0); if (!ret) { gadget->connected = 0; - gadget->udc->driver->disconnect(gadget); + mutex_lock(&udc_lock); + if (gadget->udc->driver) + gadget->udc->driver->disconnect(gadget); + mutex_unlock(&udc_lock); } out: @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev usb_gadget_udc_set_speed(udc, driver->max_speed); - mutex_lock(&udc_lock); ret = driver->bind(udc->gadget, driver); if (ret) goto err_bind; @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev goto err_start; usb_gadget_enable_async_callbacks(udc); usb_udc_connect_control(udc); - mutex_unlock(&udc_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev dev_err(&udc->dev, "failed to start %s: %d\n", driver->function, ret); + mutex_lock(&udc_lock); udc->driver = NULL; driver->is_bound = false; mutex_unlock(&udc_lock); @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - mutex_lock(&udc_lock); usb_gadget_disconnect(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq) @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct udc->driver->unbind(gadget); usb_gadget_udc_stop(udc); + mutex_lock(&udc_lock); driver->is_bound = false; udc->driver = NULL; mutex_unlock(&udc_lock); @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct struct usb_udc *udc = container_of(dev, struct usb_udc, dev); ssize_t ret; - mutex_lock(&udc_lock); + device_lock(&udc->gadget->dev); if (!udc->driver) { dev_err(dev, "soft-connect without a gadget driver\n"); ret = -EOPNOTSUPP; @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct ret = n; out: - mutex_unlock(&udc_lock); + device_unlock(&udc->gadget->dev); return ret; } static DEVICE_ATTR_WO(soft_connect); @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi char *buf) { struct usb_udc *udc = container_of(dev, struct usb_udc, dev); - struct usb_gadget_driver *drv = udc->driver; + struct usb_gadget_driver *drv; + int rc = 0; - if (!drv || !drv->function) - return 0; - return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_lock(&udc_lock); + drv = udc->driver; + if (drv && drv->function) + rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function); + mutex_unlock(&udc_lock); + return rc; } static DEVICE_ATTR_RO(function);