Fix potential shift out-of-bounds in xenhcd_hub_control()
ClearPortFeature handling and SetPortFeature handling.

wValue may be greater than 32 which can not be used for shifting.

similar patch:
https://patchwork.kernel.org/patch/12162547

Signed-off-by: Zhang Shurong <zhang_shurong@foxmail.com>
---
 drivers/usb/host/xen-hcd.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/host/xen-hcd.c b/drivers/usb/host/xen-hcd.c
index 46fdab940092..c0e7207d3857 100644
--- 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);
 			break;
 		}
@@ -527,6 +529,8 @@ static int xenhcd_hub_control(struct usb_hcd *hcd, __u16 typeReq, __u16 wValue,
 			xenhcd_rhport_suspend(info, wIndex);
 			break;
 		default:
+			if (wValue >= 32)
+				goto error;
 			if (info->ports[wIndex-1].status & USB_PORT_STAT_POWER)
 				info->ports[wIndex-1].status |= (1 << wValue);
 		}
-- 
2.41.0

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.

Jan

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

在 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);