On Mon, Dec 02, 2024 at 10:33:11AM +0800, Xu Yang wrote: > 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]; Many times, static analysis tools are just wrong :) But ok, this makes a bit more sense, can you resend this with the additional information in the changelog text? thanks, greg k-h
