Re: [PATCH] xen: fix potential shift out-of-bounds in xenhcd_hub_control()

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Sun, Aug 06, 2023 at 11:15:51AM -0400, Alan Stern wrote:
> On Sun, Aug 06, 2023 at 04:27:27PM +0200, Greg KH wrote:
> > On Sun, Aug 06, 2023 at 10:11:43PM +0800, Zhang Shurong wrote:
> > > 在 2023年7月1日星期六 CST 下午11:51:43，Zhang Shurong 写道：
> > > > 在 2023年6月26日星期一 CST 下午1:52:02，您写道：
> > > > 
> > > > > On Mon, Jun 26, 2023 at 07:48:05AM +0200, Jan Beulich wrote:
> > > > > > On 25.06.2023 18:42, Zhang Shurong wrote:
> > > > > > > --- a/drivers/usb/host/xen-hcd.c
> > > > > > > +++ b/drivers/usb/host/xen-hcd.c
> > > > > > > @@ -456,6 +456,8 @@ static int xenhcd_hub_control(struct usb_hcd 
> > > > > > > *hcd,
> > > > > > > __u16 typeReq, __u16 wValue,> >
> > > > > > > 
> > > > > > >                   info->ports[wIndex - 1].c_connection =
> > > > 
> > > > false;
> > > > 
> > > > > > >                   fallthrough;
> > > > > > >           
> > > > > > >           default:
> > > > > > > +                 if (wValue >= 32)
> > > > > > > +                         goto error;
> > > > > > > 
> > > > > > >                   info->ports[wIndex - 1].status &= ~(1
> > > > 
> > > > << wValue);
> > > > 
> > > > > > Even 31 is out of bounds (as in: UB) as long as it's 1 here rather
> > > > > > than 1u.
> > > > > 
> > > > > Why isn't the caller fixed so this type of value could never be passed
> > > > > to the hub_control callback?
> > > > > 
> > > > > thanks,
> > > > > 
> > > > > greg k-h
> > > > 
> > > > Although I'm not knowledgeable about the USB subsystem, I've observed 
> > > > that
> > > > not all driver code that implements hub_control callback performs a 
> > > > shift
> > > > operation on wValue, and not all shift operations among them cause
> > > > problems. Therefore, I've decided to fix this issue within each driver
> > > > itself.
> > > > 
> > > > For example, in r8a66597_hub_control, it will first check whether 
> > > > wValue is
> > > > valid (always < 31) before the shift operation. In case of an invalid
> > > > number, the code would execute the error branch instead of the shift
> > > > operation.
> > > > 
> > > > switch (wValue) {
> > > > case USB_PORT_FEAT_ENABLE:
> > > >         rh->port &= ~USB_PORT_STAT_POWER;
> > > >         break;
> > > > case USB_PORT_FEAT_SUSPEND:
> > > >         break;
> > > > case USB_PORT_FEAT_POWER:
> > > >         r8a66597_port_power(r8a66597, port, 0);
> > > >         break;
> > > > case USB_PORT_FEAT_C_ENABLE:
> > > > case USB_PORT_FEAT_C_SUSPEND:
> > > > case USB_PORT_FEAT_C_CONNECTION:
> > > > case USB_PORT_FEAT_C_OVER_CURRENT:
> > > > case USB_PORT_FEAT_C_RESET:
> > > >         break;
> > > > default:
> > > >         goto error;
> > > > }
> > > > rh->port &= ~(1 << wValue);
> > > 
> > > Hi there. I apologize for reaching out once more. I'm feeling a bit 
> > > puzzled 
> > > about what my next step should be. I'm unsure whether I should rewrite 
> > > this 
> > > patch or attempt to address the issue at the caller level.
> > 
> > Try addressing it at the caller level first please.  If that somehow
> > does not work, then we will take a patch series that fixes all of the
> > host controller drivers at once.
> 
> It's not feasible to fix all the callers, because the calls can come 
> from userspace via usbfs.

It can?  Hm, that happens through the call in rh_call_control(), right?
But there, we do a bunch of validation before calling hub_control() so
why can't we do the same thing in that one place as well?  Making
invalid requests from userspace should be disallowed (or we can catch
this in the usbfs interface.)

thanks,

greg k-h