| Message ID | tencent_AD4994DC28D60E6CF580E97BB028A0A1EA0A@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: r8a66597-hcd: host: fix port index underflow and UBSAN complains | expand |
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@foxmail.com> > --- > 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
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@foxmail.com> > > --- > > 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
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
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);
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@foxmail.com> --- drivers/usb/host/r8a66597-hcd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)