[PATCH] usbip: vhci_hcd: do proper error handling

[PATCH] usbip: vhci_hcd: do proper error handling

[Muhammad Usama Anjum]

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")
   
Signed-off-by: Muhammad Usama Anjum <musamaanjum@gmail.com>
---
 drivers/usb/usbip/vhci_hcd.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index 3209b5ddd30c..e32c080a2825 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -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

Re: [PATCH] usbip: vhci_hcd: do proper error handling

[Shuah Khan]

On 3/25/21 5:46 AM, Muhammad Usama Anjum wrote:
> 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.
> 

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

Re: [PATCH] usbip: vhci_hcd: do proper error handling

[Muhammad Usama Anjum]

On Fri, 2021-03-26 at 14:24 -0600, Shuah Khan wrote:
> On 3/25/21 5:46 AM, Muhammad Usama Anjum wrote:
> > 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.
> > 
> 
> Are you sure this change is necessary for vhci_hcd? Is there a
> scenario where vhci_hcd will receive these values from userspace?
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.

> Is there a way to reproduce the problem?
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.

> thanks,
> -- Shuah

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

Re: [PATCH] usbip: vhci_hcd: do proper error handling

[Shuah Khan]

On 3/31/21 5:23 AM, Muhammad Usama Anjum wrote:
> On Fri, 2021-03-26 at 14:24 -0600, Shuah Khan wrote:
>> On 3/25/21 5:46 AM, Muhammad Usama Anjum wrote:
>>> 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.
>>>
>>
>> Are you sure this change is necessary for vhci_hcd? Is there a
>> scenario where vhci_hcd will receive these values from userspace?
> 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.
> 

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.

>> Is there a way to reproduce the problem?
> 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.
> 

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

thanks,
-- Shuah