Re: [PATCH] usb: chipidea: host: Improve port index sanitizing

[Xu Yang <xu.yang_2@nxp.com>]

On Fri, Nov 29, 2024 at 01:14:35PM +0100, Greg KH wrote:
> On Fri, Nov 29, 2024 at 07:33:18PM +0800, Xu Yang wrote:
> > Coverity complains "Illegal address computation (OVERRUN)" on status_reg.
> > This will follow "846cbf98cbef USB: EHCI: Improve port index sanitizing" to
> > improve port index sanitizing.
> > 
> > Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> > ---
> >  drivers/usb/chipidea/host.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index 0cce19208370..442d79e32a65 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -256,8 +256,14 @@ static int ci_ehci_hub_control(
> >  	struct device *dev = hcd->self.controller;
> >  	struct ci_hdrc *ci = dev_get_drvdata(dev);
> >  
> > -	port_index = wIndex & 0xff;
> > -	port_index -= (port_index > 0);
> > +	/*
> > +	 * Avoid out-of-bounds values while calculating the port index
> > +	 * from wIndex. The compiler doesn't like pointers to invalid
> > +	 * addresses, even if they are never used.
> 
> The compiler does not care so what does care?  Why is this needed if it
> is never accessed?  This comment is odd to me.

I refer to Alan's comments[1]. So the compiler may report this issue on his
side. On my side, the static analysis tool is Coverity from Synopsys. It's
reporting that port_index may be bigger than HCS_N_PORTS_MAX(15). So
illegal array pointer may be caculated. 

[1] https://lore.kernel.org/r/20211002190217.GA537967@rowland.harvard.edu

> 
> thanks,
> 
> greg k-h
> 
> 
> > +	 */
> > +	port_index = (wIndex - 1) & 0xff;
> > +	if (port_index >= HCS_N_PORTS_MAX)
> > +		port_index = 0;
> >  	status_reg = &ehci->regs->port_status[port_index];
> 
> So this is used?  But what controls wIndex here and how can it be too
> big?

The wIndex stands for port number here. Actually wIndex won't be too big.
However, the static analysis tool just see:

  port_index = wIndex & 0xff;
  port_index -= (port_index > 0);

and it think the value of port_index is now between 0 and 254 (inclusive).

ehci_def.h define port_status as below:

  #define HCS_N_PORTS_MAX         15
  u32     port_status[HCS_N_PORTS_MAX];

So the tool think illegal array pointer may be obtained.

  status_reg = &ehci->regs->port_status[port_index];

Thanks,
Xu Yang

> 
> thanks,
> 
> greg k-h