[PATCH v2] USB: sisusbvga: avoid prolonged initialization on failure

[PATCH v2] USB: sisusbvga: avoid prolonged initialization on failure

[Ayush Yaduvanshi]

The USB core holds minor_rwsem while invoking a USB character
device's open callback. Avoid retrying graphics-device initialization
from open() and fail probe when early initialization fails.

Also stop sisusb_do_init_gfxdevice() after the first failed request.
Continuing initialization after a failed USB request causes additional
synchronous transfers and their retries against an unresponsive device.

v2:
Stop sisusb_do_init_gfxdevice() after the first failed USB request and
treat early initialization failure in probe() as a probe failure.
Remove the initialization retry from sisusb_open().

The exact v2 patch was tested by syzbot using the reported reproducer,
and the reproducer did not trigger the reported hang.

Reported-by: syzbot+3bc656a9271e7c8a5c6b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3bc656a9271e7c8a5c6b
Tested-by: syzbot+3bc656a9271e7c8a5c6b@syzkaller.appspotmail.com
Signed-off-by: Ayush Yaduvanshi <ayush37735@gmail.com>
---
Thanks for the review.

v2:
- Stop sisusb_do_init_gfxdevice() after the first failed USB request
  instead of continuing with further synchronous requests and their
  timeout/retry cost on an unresponsive device.
- Treat failure of the early graphics-device initialization in probe()
  as a probe failure, so a device which failed initialization is not
  left for a later initialization attempt from open().
- Remove the graphics-device initialization retry from sisusb_open().
- The exact v2 patch was tested by syzbot using the reported reproducer;
  syzbot reports that the reproducer did not trigger the reported hang.

 drivers/usb/misc/sisusbvga/sisusbvga.c | 133 +++++++++++++++++--------
 1 file changed, 89 insertions(+), 44 deletions(-)

diff --git a/drivers/usb/misc/sisusbvga/sisusbvga.c b/drivers/usb/misc/sisusbvga/sisusbvga.c
index 3e75a7c24828..e0b1111c1f53 100644
--- a/drivers/usb/misc/sisusbvga/sisusbvga.c
+++ b/drivers/usb/misc/sisusbvga/sisusbvga.c
@@ -2075,66 +2075,114 @@ static int sisusb_do_init_gfxdevice(struct sisusb_usb_data *sisusb)
 	packet.address = 0x00000324;
 	packet.data    = 0x00000004;
 	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	if (ret)
+		return ret;
 
 	packet.header  = 0x001f;
 	packet.address = 0x00000364;
 	packet.data    = 0x00000004;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	if (ret)
+		return ret;
 
 	packet.header  = 0x001f;
 	packet.address = 0x00000384;
 	packet.data    = 0x00000004;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	if (ret)
+		return ret;
 
 	packet.header  = 0x001f;
 	packet.address = 0x00000100;
 	packet.data    = 0x00000700;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	if (ret)
+		return ret;
 
 	packet.header  = 0x000f;
 	packet.address = 0x00000004;
-	ret |= sisusb_send_bridge_packet(sisusb, 6, &packet, 0);
+	ret = sisusb_send_bridge_packet(sisusb, 6, &packet, 0);
+	if (ret)
+		return ret;
+
 	packet.data |= 0x17;
-	ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	ret = sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
+	if (ret)
+		return ret;
 
 	/* Init BAR 0 (VRAM) */
-	ret |= sisusb_read_pci_config(sisusb, 0x10, &tmp32);
-	ret |= sisusb_write_pci_config(sisusb, 0x10, 0xfffffff0);
-	ret |= sisusb_read_pci_config(sisusb, 0x10, &tmp32);
+	ret = sisusb_read_pci_config(sisusb, 0x10, &tmp32);
+	if (ret)
+		return ret;
+
+	ret = sisusb_write_pci_config(sisusb, 0x10, 0xfffffff0);
+	if (ret)
+		return ret;
+
+	ret = sisusb_read_pci_config(sisusb, 0x10, &tmp32);
+	if (ret)
+		return ret;
+
 	tmp32 &= 0x0f;
 	tmp32 |= SISUSB_PCI_MEMBASE;
-	ret |= sisusb_write_pci_config(sisusb, 0x10, tmp32);
+	ret = sisusb_write_pci_config(sisusb, 0x10, tmp32);
+	if (ret)
+		return ret;
 
 	/* Init BAR 1 (MMIO) */
-	ret |= sisusb_read_pci_config(sisusb, 0x14, &tmp32);
-	ret |= sisusb_write_pci_config(sisusb, 0x14, 0xfffffff0);
-	ret |= sisusb_read_pci_config(sisusb, 0x14, &tmp32);
+	ret = sisusb_read_pci_config(sisusb, 0x14, &tmp32);
+	if (ret)
+		return ret;
+
+	ret = sisusb_write_pci_config(sisusb, 0x14, 0xfffffff0);
+	if (ret)
+		return ret;
+
+	ret = sisusb_read_pci_config(sisusb, 0x14, &tmp32);
+	if (ret)
+		return ret;
+
 	tmp32 &= 0x0f;
 	tmp32 |= SISUSB_PCI_MMIOBASE;
-	ret |= sisusb_write_pci_config(sisusb, 0x14, tmp32);
+	ret = sisusb_write_pci_config(sisusb, 0x14, tmp32);
+	if (ret)
+		return ret;
 
 	/* Init BAR 2 (i/o ports) */
-	ret |= sisusb_read_pci_config(sisusb, 0x18, &tmp32);
-	ret |= sisusb_write_pci_config(sisusb, 0x18, 0xfffffff0);
-	ret |= sisusb_read_pci_config(sisusb, 0x18, &tmp32);
+	ret = sisusb_read_pci_config(sisusb, 0x18, &tmp32);
+	if (ret)
+		return ret;
+
+	ret = sisusb_write_pci_config(sisusb, 0x18, 0xfffffff0);
+	if (ret)
+		return ret;
+
+	ret = sisusb_read_pci_config(sisusb, 0x18, &tmp32);
+	if (ret)
+		return ret;
+
 	tmp32 &= 0x0f;
 	tmp32 |= SISUSB_PCI_IOPORTBASE;
-	ret |= sisusb_write_pci_config(sisusb, 0x18, tmp32);
+	ret = sisusb_write_pci_config(sisusb, 0x18, tmp32);
+	if (ret)
+		return ret;
 
 	/* Enable memory and i/o access */
-	ret |= sisusb_read_pci_config(sisusb, 0x04, &tmp32);
+	ret = sisusb_read_pci_config(sisusb, 0x04, &tmp32);
+	if (ret)
+		return ret;
+
 	tmp32 |= 0x3;
-	ret |= sisusb_write_pci_config(sisusb, 0x04, tmp32);
-
-	if (ret == 0) {
-		/* Some further magic */
-		packet.header  = 0x001f;
-		packet.address = 0x00000050;
-		packet.data    = 0x000000ff;
-		ret |= sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
-	}
+	ret = sisusb_write_pci_config(sisusb, 0x04, tmp32);
+	if (ret)
+		return ret;
 
-	return ret;
+	/* Some further magic */
+	packet.header  = 0x001f;
+	packet.address = 0x00000050;
+	packet.data    = 0x000000ff;
+
+	return sisusb_send_bridge_packet(sisusb, 10, &packet, 0);
 }
 
 /* Initialize the graphics device (return 0 on success)
@@ -2223,20 +2271,10 @@ static int sisusb_open(struct inode *inode, struct file *file)
 	}
 
 	if (!sisusb->devinit) {
-		if (sisusb->sisusb_dev->speed == USB_SPEED_HIGH ||
-				sisusb->sisusb_dev->speed >= USB_SPEED_SUPER) {
-			if (sisusb_init_gfxdevice(sisusb, 0)) {
-				mutex_unlock(&sisusb->lock);
-				dev_err(&sisusb->sisusb_dev->dev,
-						"Failed to initialize device\n");
-				return -EIO;
-			}
-		} else {
-			mutex_unlock(&sisusb->lock);
-			dev_err(&sisusb->sisusb_dev->dev,
-					"Device not attached to USB 2.0 hub\n");
-			return -EIO;
-		}
+		mutex_unlock(&sisusb->lock);
+		dev_err(&sisusb->sisusb_dev->dev,
+			"Device not initialized\n");
+		return -EIO;
 	}
 
 	/* Increment usage count for our sisusb */
@@ -2880,9 +2918,16 @@ static int sisusb_probe(struct usb_interface *intf,
 
 	if (dev->speed == USB_SPEED_HIGH || dev->speed >= USB_SPEED_SUPER) {
 		int initscreen = 1;
-		if (sisusb_init_gfxdevice(sisusb, initscreen))
+
+		if (sisusb_init_gfxdevice(sisusb, initscreen)) {
 			dev_err(&sisusb->sisusb_dev->dev,
-					"Failed to early initialize device\n");
+				"Failed to early initialize device\n");
+			sisusb->present = 0;
+			usb_set_intfdata(intf, NULL);
+			usb_put_dev(sisusb->sisusb_dev);
+			retval = -EIO;
+			goto error_4;
+		}
 
 	} else
 		dev_info(&sisusb->sisusb_dev->dev,
-- 
2.53.0