Forwarded: [PATCH] usbip: vhci_hcd: fix UAF in attach_store() during unbind

[syzbot <syzbot+8753715f05759f1a10de@syzkaller.appspotmail.com>]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org, syzkaller-bugs@googlegroups.com.

***

Subject: [PATCH] usbip: vhci_hcd: fix UAF in attach_store() during unbind
Author: kartikey406@gmail.com

#syz test: git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master


vhci_hcd_remove() frees the SS hcd via usb_put_hcd() before the HS hcd's
vhci_stop() removes the sysfs attribute group, since the group removal is
guarded by usb_hcd_is_primary_hcd(). A concurrent write to the attach
attribute can therefore reach vhci->vhci_hcd_ss after it has been freed.

Remove the attribute group at the start of vhci_hcd_remove(), before
either hcd reference is dropped. sysfs_remove_group() drains in-flight
store callbacks, so no writer can be inside attach_store() once it
returns.

Reported-by: syzbot+8753715f05759f1a10de@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=8753715f05759f1a10de
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 drivers/usb/usbip/vhci_hcd.c | 39 +++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 39e8faf4c18c..3ce8893729b1 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -1252,18 +1252,11 @@ static int vhci_start(struct usb_hcd *hcd)
 static void vhci_stop(struct usb_hcd *hcd)
 {
 	struct vhci_hcd *vhci_hcd = hcd_to_vhci_hcd(hcd);
-	int id, rhport;
+	int rhport;
 
 	usbip_dbg_vhci_hc("stop VHCI controller\n");
 
-	/* 1. remove the userland interface of vhci_hcd */
-	id = hcd_name_to_id(hcd_name(hcd));
-	if (id == 0 && usb_hcd_is_primary_hcd(hcd)) {
-		sysfs_remove_group(&hcd_dev(hcd)->kobj, &vhci_attr_group);
-		vhci_finish_attr_group();
-	}
-
-	/* 2. shutdown all the ports of vhci_hcd */
+	/* shutdown all the ports of vhci_hcd */
 	for (rhport = 0; rhport < VHCI_HC_PORTS; rhport++) {
 		struct vhci_device *vdev = &vhci_hcd->vdev[rhport];
 
@@ -1421,20 +1414,34 @@ static int vhci_hcd_probe(struct platform_device *pdev)
 static void vhci_hcd_remove(struct platform_device *pdev)
 {
 	struct vhci *vhci = *((void **)dev_get_platdata(&pdev->dev));
+	struct usb_hcd *hcd_hs = vhci_hcd_to_hcd(vhci->vhci_hcd_hs);
+	struct usb_hcd *hcd_ss = vhci_hcd_to_hcd(vhci->vhci_hcd_ss);
+
+	/*
+	 * Remove the userland interface before dropping the hcd references.
+	 * sysfs_remove_group() waits for in-flight attach_store()/detach_store()
+	 * callers to return and rejects new ones, so neither hcd can be reached
+	 * from sysfs by the time it is freed below.
+	 */
+	if (pdev->id == 0) {
+		sysfs_remove_group(&pdev->dev.kobj, &vhci_attr_group);
+		vhci_finish_attr_group();
+	}
+
+	vhci->vhci_hcd_hs = NULL;
+	vhci->vhci_hcd_ss = NULL;
+
 
 	/*
 	 * Disconnects the root hub,
 	 * then reverses the effects of usb_add_hcd(),
 	 * invoking the HCD's stop() methods.
 	 */
-	usb_remove_hcd(vhci_hcd_to_hcd(vhci->vhci_hcd_ss));
-	usb_put_hcd(vhci_hcd_to_hcd(vhci->vhci_hcd_ss));
-
-	usb_remove_hcd(vhci_hcd_to_hcd(vhci->vhci_hcd_hs));
-	usb_put_hcd(vhci_hcd_to_hcd(vhci->vhci_hcd_hs));
+	usb_remove_hcd(hcd_ss);
+	usb_put_hcd(hcd_ss);
 
-	vhci->vhci_hcd_hs = NULL;
-	vhci->vhci_hcd_ss = NULL;
+	usb_remove_hcd(hcd_hs);
+	usb_put_hcd(hcd_hs);
 }
 
 #ifdef CONFIG_PM
-- 
2.43.0