| Message ID | 20241129113318.296073-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: chipidea: host: Improve port index sanitizing | expand |
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. 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? thanks, greg k-h
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
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@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]; 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
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. + */ + port_index = (wIndex - 1) & 0xff; + if (port_index >= HCS_N_PORTS_MAX) + port_index = 0; status_reg = &ehci->regs->port_status[port_index]; spin_lock_irqsave(&ehci->lock, flags);
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(-)