| Message ID | 20241202083453.704533-1-xu.yang_2@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 990c2a26f703f6558042d97e7abdeb3360bb6a63 |
| Headers | show |
| Series | [v2] usb: chipidea: host: Improve port index sanitizing | expand |
On 24-12-02 16:34:53, Xu Yang wrote: > Coverity from Synopsys complains "Illegal address computation (OVERRUN)" > on status_reg. > > After below code executed, > > port_index = wIndex & 0xff; > port_index -= (port_index > 0); > > the static analysis tool see the value of port_index is now between 0 and > 254 (inclusive). > > However, 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]; > > 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> Acked-by: Peter Chen <peter.chen@kernel.org> Peter > > --- > Changes in v2: > - rewrite commit message to better understand the issue > --- > 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 >
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 from Synopsys complains "Illegal address computation (OVERRUN)" on status_reg. After below code executed, port_index = wIndex & 0xff; port_index -= (port_index > 0); the static analysis tool see the value of port_index is now between 0 and 254 (inclusive). However, 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]; 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> --- Changes in v2: - rewrite commit message to better understand the issue --- drivers/usb/chipidea/host.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)