Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
To:     gregkh@linuxfoundation.org, yangyingliang@huawei.com
Cc:     linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
Subject: [PATCH] usb: max-3421: Prevent corruption of freed memory
Date:   Fri, 25 Jun 2021 15:14:56 +1200
Message-Id: <20210625031456.8632-1-mark.tomlinson@alliedtelesis.co.nz>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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 dri=
ver.")
Signed-off-by: Mark Tomlinson <mark.tomlinson@alliedtelesis.co.nz>
---
 drivers/usb/host/max3421-hcd.c | 44 +++++++++++-----------------------
 1 file changed, 14 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/host/max3421-hcd.c b/drivers/usb/host/max3421-hc=
d.c
index afd9174d83b1..abceca1c9c0f 100644
--- a/drivers/usb/host/max3421-hcd.c
+++ b/drivers/usb/host/max3421-hcd.c
@@ -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,39 +490,17 @@ max3421_set_speed(struct usb_hcd *hcd, struct usb_d=
evice *dev)
  * Caller must NOT hold HCD spinlock.
  */
 static void
-max3421_set_address(struct usb_hcd *hcd, struct usb_device *dev, int epn=
um,
-		    int force_toggles)
+max3421_set_address(struct usb_hcd *hcd, struct usb_device *dev, int epn=
um)
 {
-	struct max3421_hcd *max3421_hcd =3D hcd_to_max3421(hcd);
-	int old_epnum, same_ep, rcvtog, sndtog;
-	struct usb_device *old_dev;
+	int rcvtog, sndtog;
 	u8 hctl;
=20
-	old_dev =3D max3421_hcd->loaded_dev;
-	old_epnum =3D max3421_hcd->loaded_epnum;
-
-	same_ep =3D (dev =3D=3D old_dev && epnum =3D=3D old_epnum);
-	if (same_ep && !force_toggles)
-		return;
-
-	if (old_dev && !same_ep) {
-		/* save the old end-points toggles: */
-		u8 hrsl =3D spi_rd8(hcd, MAX3421_REG_HRSL);
-
-		rcvtog =3D (hrsl >> MAX3421_HRSL_RCVTOGRD_BIT) & 1;
-		sndtog =3D (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 =3D usb_gettoggle(dev, epnum, 0);
 	sndtog =3D usb_gettoggle(dev, epnum, 1);
 	hctl =3D (BIT(rcvtog + MAX3421_HCTL_RCVTOG0_BIT) |
 		BIT(sndtog + MAX3421_HCTL_SNDTOG0_BIT));
=20
-	max3421_hcd->loaded_epnum =3D epnum;
 	spi_wr8(hcd, MAX3421_REG_HCTL, hctl);
=20
 	/*
@@ -532,7 +508,6 @@ max3421_set_address(struct usb_hcd *hcd, struct usb_d=
evice *dev, int epnum,
 	 * address-assignment so it's best to just always load the
 	 * address whenever the end-point changed/was forced.
 	 */
-	max3421_hcd->loaded_dev =3D dev;
 	spi_wr8(hcd, MAX3421_REG_PERADDR, dev->devnum);
 }
=20
@@ -667,7 +642,7 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
 	struct max3421_hcd *max3421_hcd =3D hcd_to_max3421(hcd);
 	struct urb *urb, *curr_urb =3D NULL;
 	struct max3421_ep *max3421_ep;
-	int epnum, force_toggles =3D 0;
+	int epnum;
 	struct usb_host_endpoint *ep;
 	struct list_head *pos;
 	unsigned long flags;
@@ -777,7 +752,6 @@ 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 =3D PKT_STATE_SETUP;
-			force_toggles =3D 1;
 		} else
 			max3421_ep->pkt_state =3D PKT_STATE_TRANSFER;
 	}
@@ -785,7 +759,7 @@ max3421_select_and_start_urb(struct usb_hcd *hcd)
 	spin_unlock_irqrestore(&max3421_hcd->lock, flags);
=20
 	max3421_ep->last_active =3D 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;
@@ -1380,6 +1354,16 @@ max3421_urb_done(struct usb_hcd *hcd)
 		status =3D 0;
 	urb =3D max3421_hcd->curr_urb;
 	if (urb) {
+		/* save the old end-points toggles: */
+		u8 hrsl =3D spi_rd8(hcd, MAX3421_REG_HRSL);
+		int rcvtog =3D (hrsl >> MAX3421_HRSL_RCVTOGRD_BIT) & 1;
+		int sndtog =3D (hrsl >> MAX3421_HRSL_SNDTOGRD_BIT) & 1;
+		int epnum =3D 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 =3D NULL;
 		spin_lock_irqsave(&max3421_hcd->lock, flags);
 		usb_hcd_unlink_urb_from_ep(hcd, urb);
--=20
2.32.0