CVE-2021-38204: Use After Free (CWE-416)
value: drivers/usb/host/max3421-hcd.c in the Linux kernel before 5.13.6 allows physically proximate attackers to cause a denial of service (use-after-free and panic) by removing a MAX-3421 USB device in certain situations.
CWE description: The product reuses or references memory after it has been freed. At some point afterward, the memory may be allocated again and saved in another pointer, while the original pointer references a location somewhere within the new allocation. Any operations using the original pointer are no longer valid because the memory "belongs" to the code that operates on the new pointer.
CVSS v3 base score: 6.8
Published: 2021-08-08T20:15Z
Repository: https://github.com/torvalds/linux
Language: C
Files changed: drivers/usb/host/max3421-hcd.c
Commit date: 2021-07-21 09:47:15 +0200
Commit message: usb: max-3421: Prevent corruption of freed memory

The MAX-3421 USB driver remembers the state of the USB toggles for a
device/endpoint. To save SPI writes, this was only done when a new
device/endpoint was being used. Unfortunately, if the old device was
removed, this would cause writes to freed memory.

To fix this, a simpler scheme is used. The toggles are read from
hardware when a URB is completed, and the toggles are always written to
hardware when any URB transaction is started. This will cause a few more
SPI transactions, but no causes kernel panics.

Fixes: 2d53139f3162 ("Add support for using a MAX3421E chip as a host driver.")
Cc: stable <stable@vger.kernel.org>
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
Link: https://lore.kernel.org/r/20210625031456.8632-1-mark.tomlinson@alliedtelesis.co.nz
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Diff: diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hcd.c
index e7a8e0609853..59cc1bc7f12f 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -151,12 +151,10 @@ struct max3421_hcd {
 	 * URB we're currently processing.  Must not be reset to NULL
 	 * unless MAX3421E chip is idle:
 	 */
 	struct urb *curr_urb;
 	enum scheduling_pass sched_pass;
-	struct usb_device *loaded_dev;	/* dev that's loaded into the chip */
-	int loaded_epnum;		/* epnum whose toggles are loaded */
 	int urb_done;			/* > 0 -> no errors, < 0: errno */
 	size_t curr_len;
 	u8 hien;
 	u8 mode;
 	u8 iopins[2];
@@ -490,51 +488,28 @@ max3421_set_speed(struct usb_hcd *hcd, struct usb_device *dev)
 
 /*
  * Caller must NOT hold HCD spinlock.
  */
 static void
-max3421_set_address(struct usb_hcd *hcd, struct usb_device *dev, int epnum,
-		    int force_toggles)
+max3421_set_address(struct usb_hcd *hcd, struct usb_device *dev, int epnum)
 {
-	struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
-	int old_epnum, same_ep, rcvtog, sndtog;
-	struct usb_device *old_dev;
+	int rcvtog, sndtog;
 	u8 hctl;
 
-	old_dev = max3421_hcd->loaded_dev;
-	old_epnum = max3421_hcd->loaded_epnum;
-
-	same_ep = (dev == old_dev && epnum == old_epnum);
-	if (same_ep && !force_toggles)
-		return;
-
-	if (old_dev && !same_ep) {
-		/* save the old end-points toggles: */
-		u8 hrsl = spi_rd8(hcd, MAX3421_REG_HRSL);
-
-		rcvtog = (hrsl >> MAX3421_HRSL_RCVTOGRD_BIT) & 1;
-		sndtog = (hrsl >> MAX3421_HRSL_SNDTOGRD_BIT) & 1;
-
-		/* no locking: HCD (i.e., we) own toggles, don't we? */
-		usb_settoggle(old_dev, old_epnum, 0, rcvtog);
-		usb_settoggle(old_dev, old_epnum, 1, sndtog);
-	}
 	/* setup new endpoint's toggle bits: */
 	rcvtog = usb_gettoggle(dev, epnum, 0);
 	sndtog = usb_gettoggle(dev, epnum, 1);
 	hctl = (BIT(rcvtog + MAX3421_HCTL_RCVTOG0_BIT) |
 		BIT(sndtog + MAX3421_HCTL_SNDTOG0_BIT));
 
-	max3421_hcd->loaded_epnum = epnum;
 	spi_wr8(hcd, MAX3421_REG_HCTL, hctl);
 
 	/*
 	 * Note: devnum for one and the same device can change during
 	 * address-assignment so it's best to just always load the
 	 * address whenever the end-point changed/was forced.
 	 */
-	max3421_hcd->loaded_dev = dev;
 	spi_wr8(hcd, MAX3421_REG_PERADDR, dev->devnum);
 }
 
 static int
 max3421_ctrl_setup(struct usb_hcd *hcd, struct urb *urb)
@@ -665,11 +640,11 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
 {
 	struct spi_device *spi = to_spi_device(hcd->self.controller);
 	struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
 	struct urb *urb, *curr_urb = NULL;
 	struct max3421_ep *max3421_ep;
-	int epnum, force_toggles = 0;
+	int epnum;
 	struct usb_host_endpoint *ep;
 	struct list_head *pos;
 	unsigned long flags;
 
 	spin_lock_irqsave(&max3421_hcd->lock, flags);
@@ -775,19 +750,18 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
 			 * Initialization via SETUP Token:
 			 */
 			usb_settoggle(urb->dev, epnum, 0, 1);
 			usb_settoggle(urb->dev, epnum, 1, 1);
 			max3421_ep->pkt_state = PKT_STATE_SETUP;
-			force_toggles = 1;
 		} else
 			max3421_ep->pkt_state = PKT_STATE_TRANSFER;
 	}
 
 	spin_unlock_irqrestore(&max3421_hcd->lock, flags);
 
 	max3421_ep->last_active = max3421_hcd->frame_number;
-	max3421_set_address(hcd, urb->dev, epnum, force_toggles);
+	max3421_set_address(hcd, urb->dev, epnum);
 	max3421_set_speed(hcd, urb->dev);
 	max3421_next_transfer(hcd, 0);
 	return 1;
 }
 
@@ -1377,10 +1351,20 @@ max3421_urb_done(struct usb_hcd *hcd)
 	max3421_hcd->urb_done = 0;
 	if (status > 0)
 		status = 0;
 	urb = max3421_hcd->curr_urb;
 	if (urb) {
+		/* save the old end-points toggles: */
+		u8 hrsl = spi_rd8(hcd, MAX3421_REG_HRSL);
+		int rcvtog = (hrsl >> MAX3421_HRSL_RCVTOGRD_BIT) & 1;
+		int sndtog = (hrsl >> MAX3421_HRSL_SNDTOGRD_BIT) & 1;
+		int epnum = usb_endpoint_num(&urb->ep->desc);
+
+		/* no locking: HCD (i.e., we) own toggles, don't we? */
+		usb_settoggle(urb->dev, epnum, 0, rcvtog);
+		usb_settoggle(urb->dev, epnum, 1, sndtog);
+
 		max3421_hcd->curr_urb = NULL;
 		spin_lock_irqsave(&max3421_hcd->lock, flags);
 		usb_hcd_unlink_urb_from_ep(hcd, urb);
 		spin_unlock_irqrestore(&max3421_hcd->lock, flags);
 

Vulnerable code: 	struct usb_device *loaded_dev;	/* dev that's loaded into the chip */
	int loaded_epnum;		/* epnum whose toggles are loaded */
max3421_set_address(struct usb_hcd *hcd, struct usb_device *dev, int epnum,
		    int force_toggles)
	struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd);
	int old_epnum, same_ep, rcvtog, sndtog;
	struct usb_device *old_dev;
	old_dev = max3421_hcd->loaded_dev;
	old_epnum = max3421_hcd->loaded_epnum;

	same_ep = (dev == old_dev && epnum == old_epnum);
	if (same_ep && !force_toggles)
		return;

	if (old_dev && !same_ep) {
		/* save the old end-points toggles: */
		u8 hrsl = spi_rd8(hcd, MAX3421_REG_HRSL);

		rcvtog = (hrsl >> MAX3421_HRSL_RCVTOGRD_BIT) & 1;
		sndtog = (hrsl >> MAX3421_HRSL_SNDTOGRD_BIT) & 1;

		/* no locking: HCD (i.e., we) own toggles, don't we? */
		usb_settoggle(old_dev, old_epnum, 0, rcvtog);
		usb_settoggle(old_dev, old_epnum, 1, sndtog);
	}
	max3421_hcd->loaded_epnum = epnum;
	max3421_hcd->loaded_dev = dev;
	int epnum, force_toggles = 0;
			force_toggles = 1;
	max3421_set_address(hcd, urb->dev, epnum, force_toggles);
Fixed code: max3421_set_address(struct usb_hcd *hcd, struct usb_device *dev, int epnum)
	int rcvtog, sndtog;
	int epnum;
	max3421_set_address(hcd, urb->dev, epnum);
		/* save the old end-points toggles: */
		u8 hrsl = spi_rd8(hcd, MAX3421_REG_HRSL);
		int rcvtog = (hrsl >> MAX3421_HRSL_RCVTOGRD_BIT) & 1;
		int sndtog = (hrsl >> MAX3421_HRSL_SNDTOGRD_BIT) & 1;
		int epnum = usb_endpoint_num(&urb->ep->desc);

		/* no locking: HCD (i.e., we) own toggles, don't we? */
		usb_settoggle(urb->dev, epnum, 0, rcvtog);
		usb_settoggle(urb->dev, epnum, 1, sndtog);