Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] usbip: vhci_hcd: do proper error handling
@ 2021-03-25 11:46 Muhammad Usama Anjum
  2021-03-26 20:24 ` Shuah Khan
  0 siblings, 1 reply; 4+ messages in thread
From: Muhammad Usama Anjum @ 2021-03-25 11:46 UTC (permalink / raw)
  To: shuah, gregkh, linux-kernel, linux-usb, valentina.manea.m, stern
  Cc: musamaanjum

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: do proper error handling
  2021-03-25 11:46 [PATCH] usbip: vhci_hcd: do proper error handling Muhammad Usama Anjum
@ 2021-03-26 20:24 ` Shuah Khan
  2021-03-31 11:23   ` Muhammad Usama Anjum
  0 siblings, 1 reply; 4+ messages in thread
From: Shuah Khan @ 2021-03-26 20:24 UTC (permalink / raw)
  To: Muhammad Usama Anjum, shuah, gregkh, linux-kernel, linux-usb,
	valentina.manea.m, stern, 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: do proper error handling
  2021-03-26 20:24 ` Shuah Khan
@ 2021-03-31 11:23   ` Muhammad Usama Anjum
  2021-04-08 15:44     ` Shuah Khan
  0 siblings, 1 reply; 4+ messages in thread
From: Muhammad Usama Anjum @ 2021-03-31 11:23 UTC (permalink / raw)
  To: Shuah Khan, shuah, gregkh, linux-kernel, linux-usb,
	valentina.manea.m, stern
  Cc: musamaanjum

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] usbip: vhci_hcd: do proper error handling
  2021-03-31 11:23   ` Muhammad Usama Anjum
@ 2021-04-08 15:44     ` Shuah Khan
  0 siblings, 0 replies; 4+ messages in thread
From: Shuah Khan @ 2021-04-08 15:44 UTC (permalink / raw)
  To: Muhammad Usama Anjum, shuah, gregkh, linux-kernel, linux-usb,
	valentina.manea.m, stern, 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

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, back to index

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-25 11:46 [PATCH] usbip: vhci_hcd: do proper error handling Muhammad Usama Anjum
2021-03-26 20:24 ` Shuah Khan
2021-03-31 11:23   ` Muhammad Usama Anjum
2021-04-08 15:44     ` Shuah Khan

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git