Vulnerability: 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.
Vulnerability ID: CVE-2021-38204
CWE: CWE-416
Severity score: 6.4
Project: https://github.com/torvalds/linux
Published: 2021-08-08
File: drivers/usb/host/max3421-hcd.c
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>
Commit author: Mark Tomlinson

Vulnerable version (before fix):
 struct max3421_hcd { 	 */ 	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; 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) { 	struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd); 	int old_epnum, same_ep, rcvtog, sndtog; 	struct usb_device *old_dev; 	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); }  max3421_select_and_start_urb(struct usb_hcd *hcd) 	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; 	struct usb_host_endpoint *ep; 	struct list_head *pos; 	unsigned long flags; max3421_select_and_start_urb(struct usb_hcd *hcd) 			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_speed(hcd, urb->dev); 	max3421_next_transfer(hcd, 0); 	return 1; max3421_urb_done(struct usb_hcd *hcd) 		status = 0; 	urb = max3421_hcd->curr_urb; 	if (urb) { 		max3421_hcd->curr_urb = NULL; 		spin_lock_irqsave(&max3421_hcd->lock, flags); 		usb_hcd_unlink_urb_from_ep(hcd, urb);

Fixed version (after fix):
 struct max3421_hcd { 	 */ 	struct urb *curr_urb; 	enum scheduling_pass sched_pass; 	int urb_done;			/* > 0 -> no errors, < 0: errno */ 	size_t curr_len; 	u8 hien; 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 rcvtog, sndtog; 	u8 hctl;  	/* 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));  	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. 	 */ 	spi_wr8(hcd, MAX3421_REG_PERADDR, dev->devnum); }  max3421_select_and_start_urb(struct usb_hcd *hcd) 	struct max3421_hcd *max3421_hcd = hcd_to_max3421(hcd); 	struct urb *urb, *curr_urb = NULL; 	struct max3421_ep *max3421_ep; 	int epnum; 	struct usb_host_endpoint *ep; 	struct list_head *pos; 	unsigned long flags; max3421_select_and_start_urb(struct usb_hcd *hcd) 			usb_settoggle(urb->dev, epnum, 0, 1); 			usb_settoggle(urb->dev, epnum, 1, 1); 			max3421_ep->pkt_state = PKT_STATE_SETUP; 		} 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); 	max3421_set_speed(hcd, urb->dev); 	max3421_next_transfer(hcd, 0); 	return 1; max3421_urb_done(struct usb_hcd *hcd) 		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);

Patch:
patch: @@ -153,8 +153,6 @@ struct max3421_hcd {
 	 */
 	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;
@@ -492,47 +490,24 @@ 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);
 }
 
@@ -667,7 +642,7 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
 	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;
@@ -777,15 +752,14 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
 			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;
@@ -1379,6 +1353,16 @@ max3421_urb_done(struct usb_hcd *hcd)
 		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);
path: drivers/usb/host/max3421-hcd.c