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@xxxxxxx> > > --- > > 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@xxxxxxxxxxxxxxxxxxx > > 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
