usbip: vhci_hcd: do proper error handling

The driver was assuming that all the parameters would be valid. But it is possible that parameters are sent from userspace. For those cases, appropriate error checks have been added.

Porting partial fix from:
c318840fb2 ("USB: Gadget: dummy-hcd: Fix shift-out-of-bounds bug")

---
 drivers/usb/usbip/vhci_hcd.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

index 3209b5ddd30c..e32c080a2825 100644
@@ -393,13 +393,24 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			else
 				vhci_hcd->port_status[rhport] &= ~USB_PORT_STAT_POWER;
 			break;
-		default:
-			usbip_dbg_vhci_rh(" ClearPortFeature: default %x\n",
-					  wValue);
+		case USB_PORT_FEAT_ENABLE:
+		case USB_PORT_FEAT_C_ENABLE:
+		case USB_PORT_FEAT_C_SUSPEND:
+			/* Not allowed for USB-3 */
+			if (hcd->speed == HCD_USB3)
+				goto error;
+			fallthrough;
+		case USB_PORT_FEAT_C_CONNECTION:
+		case USB_PORT_FEAT_C_RESET:
 			if (wValue >= 32)
 				goto error;
 			vhci_hcd->port_status[rhport] &= ~(1 << wValue);
 			break;
+		default:
+			/* Disallow INDICATOR and C_OVER_CURRENT */
+			usbip_dbg_vhci_rh(" ClearPortFeature: default %x\n",
+					  wValue);
+			goto error;
 		}
 		break;
 	case GetHubDescriptor:

@@ -587,6 +598,14 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 			/* 50msec reset signaling */
 			vhci_hcd->re_timeout = jiffies + msecs_to_jiffies(50);
 			fallthrough;
+		case USB_PORT_FEAT_C_CONNECTION:
+		case USB_PORT_FEAT_C_RESET:
+		case USB_PORT_FEAT_C_ENABLE:
+		case USB_PORT_FEAT_C_SUSPEND:
+			/* Not allowed for USB-3, and ignored for USB-2 */
+			if (hcd->speed == HCD_USB3)
+				goto error;
+			break;
 		default:
 			usbip_dbg_vhci_rh(" SetPortFeature: default %d\n",
 					  wValue);

2.25.1

Are you sure this change is necessary for vhci_hcd? Is there a scenario where vhci_hcd will receive these values from userspace?

Is there a way to reproduce the problem?

thanks,
-- Shuah

I'm not sure if these changes are necessary for vhci_hcd. I'd sent this patch following the motivation of a patch (c318840fb2) from dummy_hcd to handle some cases. Yeah, there is scenario where vhci_hcd will receive these values from userspace. For example, typReq = SetPortFeature and wValue = USB_PORT_FEAT_C_CONNECTION can be received from userspace. As USB_PORT_FEAT_C_CONNECTION case isn't being handled, default case will is executed for it. So I'm assuming USB_PORT_FEAT_C_CONNECTION isn't supported and default case shouldn't be executed.

I'm not able to reproduce any problem. But typReq = SetPortFeature and wValue = USB_PORT_FEAT_C_CONNECTION may trigger some behavior which isn't intended as USB_PORT_FEAT_C_CONNECTION may not be supported for vhci_hcd.

There is one line wrong in this patch. If we decide to proceed, I'll send a v2. Please let me know your thoughts.

Thanks,
Usama

The way dummy_hcd handles USB_PORT_FEAT_C_CONNECTION isn't applicable to vhci_hcd.

rh_port_connect() and rh_port_disconnect() set the USB_PORT_FEAT_C_CONNECTION this flag to initiate port status polling. This flag is set by the driver as a result of attach/deatch request
from the user-space. Current handling of this flag is correct.

If you reproduce the problem and it impacts attach/detach and device remote device access, we can to look into this further.

thanks,
-- Shuah