If wIndex is 0 (and it often is), these calculations underflow and
UBSAN complains, here resolve this by not decrementing the index when
it is equal to 0.
Similar commit 85e3990bea49 ("USB: EHCI: avoid undefined pointer
arithmetic and placate UBSAN")
The changes in this version:
- fix some compile error
Signed-off-by: Zhang Shurong <[email protected]>
---
drivers/usb/host/r8a66597-hcd.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
index 9f4bf8c5f8a5..6c597c668364 100644
--- a/drivers/usb/host/r8a66597-hcd.c
+++ b/drivers/usb/host/r8a66597-hcd.c
@@ -2141,10 +2141,12 @@ static int r8a66597_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
{
struct r8a66597 *r8a66597 = hcd_to_r8a66597(hcd);
int ret;
- int port = (wIndex & 0x00FF) - 1;
- struct r8a66597_root_hub *rh = &r8a66597->root_hub[port];
unsigned long flags;
+ struct r8a66597_root_hub *rh;
+ u32 port = wIndex & 0xFF;
+ port -= (port > 0);
+ rh = &r8a66597->root_hub[port];
ret = 0;
spin_lock_irqsave(&r8a66597->lock, flags);
--
2.30.2
On Sun, Jul 02, 2023 at 12:39:20AM +0800, Zhang Shurong wrote:
> If wIndex is 0 (and it often is), these calculations underflow and
> UBSAN complains, here resolve this by not decrementing the index when
> it is equal to 0.
>
> Similar commit 85e3990bea49 ("USB: EHCI: avoid undefined pointer
> arithmetic and placate UBSAN")
>
> The changes in this version:
> - fix some compile error
>
> Signed-off-by: Zhang Shurong <[email protected]>
> ---
> drivers/usb/host/r8a66597-hcd.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
> index 9f4bf8c5f8a5..6c597c668364 100644
> --- a/drivers/usb/host/r8a66597-hcd.c
> +++ b/drivers/usb/host/r8a66597-hcd.c
> @@ -2141,10 +2141,12 @@ static int r8a66597_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> {
> struct r8a66597 *r8a66597 = hcd_to_r8a66597(hcd);
> int ret;
> - int port = (wIndex & 0x00FF) - 1;
> - struct r8a66597_root_hub *rh = &r8a66597->root_hub[port];
> unsigned long flags;
> + struct r8a66597_root_hub *rh;
> + u32 port = wIndex & 0xFF;
>
> + port -= (port > 0);
I have no idea about this hardware, but it seems strange to me that
calling r8a66597_hub_control with wIndex = 1 should have the same effect
as with wIndex = 0. Is you changed backed by knowledge about the
hardware, or is that just the most obvious way to get rid of the UB
warning?
Having said that, I think
port -= (port > 0);
is hard to read compared to:
if (port > 0)
port--;
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Sat, Jul 01, 2023 at 07:16:48PM +0200, Uwe Kleine-K?nig wrote:
> On Sun, Jul 02, 2023 at 12:39:20AM +0800, Zhang Shurong wrote:
> > If wIndex is 0 (and it often is), these calculations underflow and
> > UBSAN complains, here resolve this by not decrementing the index when
> > it is equal to 0.
> >
> > Similar commit 85e3990bea49 ("USB: EHCI: avoid undefined pointer
> > arithmetic and placate UBSAN")
> >
> > The changes in this version:
> > - fix some compile error
> >
> > Signed-off-by: Zhang Shurong <[email protected]>
> > ---
> > drivers/usb/host/r8a66597-hcd.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/r8a66597-hcd.c b/drivers/usb/host/r8a66597-hcd.c
> > index 9f4bf8c5f8a5..6c597c668364 100644
> > --- a/drivers/usb/host/r8a66597-hcd.c
> > +++ b/drivers/usb/host/r8a66597-hcd.c
> > @@ -2141,10 +2141,12 @@ static int r8a66597_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
> > {
> > struct r8a66597 *r8a66597 = hcd_to_r8a66597(hcd);
> > int ret;
> > - int port = (wIndex & 0x00FF) - 1;
> > - struct r8a66597_root_hub *rh = &r8a66597->root_hub[port];
> > unsigned long flags;
> > + struct r8a66597_root_hub *rh;
> > + u32 port = wIndex & 0xFF;
> >
> > + port -= (port > 0);
>
> I have no idea about this hardware, but it seems strange to me that
> calling r8a66597_hub_control with wIndex = 1 should have the same effect
> as with wIndex = 0. Is you changed backed by knowledge about the
> hardware, or is that just the most obvious way to get rid of the UB
> warning?
>
> Having said that, I think
>
> port -= (port > 0);
>
> is hard to read compared to:
>
> if (port > 0)
> port--;
Zhang:
Why not just copy the code that's already in ehci-hub.c?
/*
* 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 = (wIndex - 1) & 0xff;
if (port >= r8a66597->max_root_hub)
port = 0;
rh = &r8a66597->root_hub[port];
Also, I see that in the ClearPortFeature, SetPortStatus, and
SetPortFeature cases in this routine, the code doesn't check for wIndex
== 0. That's a bug -- a real one, not just a UBSAN issue.
Uwe:
wIndex should never be == 0 or > max_root_hub in the cases where rh gets
used; such values would be meaningless. But we don't control the value
of wIndex, because it can come from userspace. So we can't simply
assume it will always be valid; it has to be checked.
That being understood, the changes Zhang is making here are meant mostly
to prevent UBSAN and the compiler from complaining or making false
assumptions. The actual checks on wIndex occur later in the subroutine.
Alan Stern
Hello Alan,
On Sat, Jul 01, 2023 at 02:54:46PM -0400, Alan Stern wrote:
> wIndex should never be == 0 or > max_root_hub in the cases where rh gets
> used; such values would be meaningless. But we don't control the value
> of wIndex, because it can come from userspace. So we can't simply
> assume it will always be valid; it has to be checked.
>
> That being understood, the changes Zhang is making here are meant mostly
> to prevent UBSAN and the compiler from complaining or making false
> assumptions. The actual checks on wIndex occur later in the subroutine.
I'm guilty of not having looked at all on that function, but it sounds
wrong to me to calculate values from some untrusted input and only
later validate the input. It should be the other way round, shouldn't
it? This is calling for compiler optimisations stepping on your toes.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Sun, Jul 02, 2023 at 12:19:11AM +0200, Uwe Kleine-K?nig wrote:
> Hello Alan,
>
> On Sat, Jul 01, 2023 at 02:54:46PM -0400, Alan Stern wrote:
> > wIndex should never be == 0 or > max_root_hub in the cases where rh gets
> > used; such values would be meaningless. But we don't control the value
> > of wIndex, because it can come from userspace. So we can't simply
> > assume it will always be valid; it has to be checked.
> >
> > That being understood, the changes Zhang is making here are meant mostly
> > to prevent UBSAN and the compiler from complaining or making false
> > assumptions. The actual checks on wIndex occur later in the subroutine.
>
> I'm guilty of not having looked at all on that function, but it sounds
> wrong to me to calculate values from some untrusted input and only
> later validate the input. It should be the other way round, shouldn't
> it? This is calling for compiler optimisations stepping on your toes.
Six of one, half a dozen of the other. In the end I don't think it
makes much difference; it basically comes down to personal choice.
Which is fine, provided the final choice is one of the correct ones.
Alan Stern