[v2] usb: chipidea: host: fix port index underflow and UBSAN complains

Commit Message

Jun Li June 18, 2021, 7:04 a.m. UTC

If wIndex is 0 (and it often is), these calculations underflow and
UBSAN complains, here resolve this by not decrementing the index when
it is equal to 0, this copies the solution from commit 85e3990bea49
("USB: EHCI: avoid undefined pointer arithmetic and placate UBSAN")

Reported-by: zhipeng.wang <[e-mail]>
Signed-off-by: Li Jun <[e-mail]>
---
Change for v2:
-  Add wIndex range check to ensure a correct port index value.

 drivers/usb/chipidea/host.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Greg KH June 18, 2021, 7:45 a.m. UTC | #1

On Fri, Jun 18, 2021 at 03:04:39PM +0800, Li Jun wrote:
> If wIndex is 0 (and it often is), these calculations underflow and
> UBSAN complains, here resolve this by not decrementing the index when
> it is equal to 0, this copies the solution from commit 85e3990bea49
> ("USB: EHCI: avoid undefined pointer arithmetic and placate UBSAN")
> 
> Reported-by: zhipeng.wang <[e-mail]>

Please use a name here that makes sense, "Zhipeng Wang"?

thanks,

greg k-h

Greg KH June 18, 2021, 7:57 a.m. UTC | #2

On Fri, Jun 18, 2021 at 09:45:09AM +0200, Greg KH wrote:
> On Fri, Jun 18, 2021 at 03:04:39PM +0800, Li Jun wrote:
> > If wIndex is 0 (and it often is), these calculations underflow and
> > UBSAN complains, here resolve this by not decrementing the index when
> > it is equal to 0, this copies the solution from commit 85e3990bea49
> > ("USB: EHCI: avoid undefined pointer arithmetic and placate UBSAN")
> > 
> > Reported-by: zhipeng.wang <[e-mail]>
> 
> Please use a name here that makes sense, "Zhipeng Wang"?

Specifically, the "." in the name as written originally does not make
sense, right?

Jun Li June 18, 2021, 8:40 a.m. UTC | #3

> -----Original Message-----
> From: Greg KH <[e-mail]>
> Sent: Friday, June 18, 2021 3:58 PM
> To: Jun Li <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; dl-linux-imx <[e-mail]>; Zhipeng Wang
> <[e-mail]>
> Subject: Re: [PATCH v2] usb: chipidea: host: fix port index underflow and
> UBSAN complains
> 
> On Fri, Jun 18, 2021 at 09:45:09AM +0200, Greg KH wrote:
> > On Fri, Jun 18, 2021 at 03:04:39PM +0800, Li Jun wrote:
> > > If wIndex is 0 (and it often is), these calculations underflow and
> > > UBSAN complains, here resolve this by not decrementing the index
> > > when it is equal to 0, this copies the solution from commit
> > > 85e3990bea49
> > > ("USB: EHCI: avoid undefined pointer arithmetic and placate UBSAN")
> > >
> > > Reported-by: zhipeng.wang <[e-mail]>
> >
> > Please use a name here that makes sense, "Zhipeng Wang"?
> 
> Specifically, the "." in the name as written originally does not make sense,
> right?

Yes, should be "Zhipeng Wang", I will change and send v3.

Thanks
Li Jun

Patch

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index e86d13c04bdb..bdc3885c0d49 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -240,15 +240,18 @@  static int ci_ehci_hub_control(
 )
 {
 	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
+	unsigned int	ports = HCS_N_PORTS(ehci->hcs_params);
 	u32 __iomem	*status_reg;
-	u32		temp;
+	u32		temp, port_index;
 	unsigned long	flags;
 	int		retval = 0;
 	bool		done = false;
 	struct device *dev = hcd->self.controller;
 	struct ci_hdrc *ci = dev_get_drvdata(dev);
 
-	status_reg = &ehci->regs->port_status[(wIndex & 0xff) - 1];
+	port_index = wIndex & 0xff;
+	port_index -= (port_index > 0);
+	status_reg = &ehci->regs->port_status[port_index];
 
 	spin_lock_irqsave(&ehci->lock, flags);
 
@@ -260,6 +263,11 @@  static int ci_ehci_hub_control(
 	}
 
 	if (typeReq == SetPortFeature && wValue == USB_PORT_FEAT_SUSPEND) {
+		if (!wIndex || wIndex > ports) {
+			retval = -EPIPE;
+			goto done;
+		}
+
 		temp = ehci_readl(ehci, status_reg);
 		if ((temp & PORT_PE) == 0 || (temp & PORT_RESET) != 0) {
 			retval = -EPIPE;
@@ -288,7 +296,7 @@  static int ci_ehci_hub_control(
 			ehci_writel(ehci, temp, status_reg);
 		}
 
-		set_bit((wIndex & 0xff) - 1, &ehci->suspended_ports);
+		set_bit(port_index, &ehci->suspended_ports);
 		goto done;
 	}