| Message ID | 20210325114638.GA659438@LEGION |
|---|---|
| State | New |
| Headers | show |
| Series | usbip: vhci_hcd: do proper error handling | expand |
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
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
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
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);
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(-)