Re: [PATCH v2] usb: r8a66597-hcd: host: fix port index underflow and UBSAN complains

[Alan Stern]

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 <zhang_shurong@xxxxxxxxxxx>
> > ---
> >  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