* [PATCH] usb: chipidea: host: Improve port index sanitizing @ 2024-11-29 11:33 Xu Yang 2024-11-29 12:14 ` Greg KH 0 siblings, 1 reply; 4+ messages in thread From: Xu Yang @ 2024-11-29 11:33 UTC (permalink / raw) To: peter.chen, gregkh; +Cc: linux-usb, imx, jun.li 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. + */ + 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: chipidea: host: Improve port index sanitizing 2024-11-29 11:33 [PATCH] usb: chipidea: host: Improve port index sanitizing Xu Yang @ 2024-11-29 12:14 ` Greg KH 2024-12-02 2:33 ` Xu Yang 0 siblings, 1 reply; 4+ messages in thread From: Greg KH @ 2024-11-29 12:14 UTC (permalink / raw) To: Xu Yang; +Cc: peter.chen, linux-usb, imx, jun.li 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: chipidea: host: Improve port index sanitizing 2024-11-29 12:14 ` Greg KH @ 2024-12-02 2:33 ` Xu Yang 2024-12-02 6:32 ` Greg KH 0 siblings, 1 reply; 4+ messages in thread From: Xu Yang @ 2024-12-02 2:33 UTC (permalink / raw) To: Greg KH; +Cc: peter.chen, linux-usb, imx, jun.li 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] usb: chipidea: host: Improve port index sanitizing 2024-12-02 2:33 ` Xu Yang @ 2024-12-02 6:32 ` Greg KH 0 siblings, 0 replies; 4+ messages in thread From: Greg KH @ 2024-12-02 6:32 UTC (permalink / raw) To: Xu Yang; +Cc: peter.chen, linux-usb, imx, jun.li 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-12-02 6:32 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-29 11:33 [PATCH] usb: chipidea: host: Improve port index sanitizing Xu Yang 2024-11-29 12:14 ` Greg KH 2024-12-02 2:33 ` Xu Yang 2024-12-02 6:32 ` Greg KH
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).