usbip: vhci_hcd: Check rhport everywhere in vhci_hub_control()

usbip: vhci_hcd: Check rhport everywhere in vhci_hub_control()

[Shuah Khan]

Hi Ben,

Thanks for the patch.

On 10/10/2018 11:30 PM, Ben Hutchings wrote:
> Commit 5b22f676118f "usbip: vhci_hcd: check rhport before using in
> vhci_hub_control()" added some validation of rhport, but left
> several problems:
> 
> - If VHCI_HC_PORTS < 256, we can get rhport >= VHCI_HC_PORTS which
>   is also out of range.  To keep things simple, set rhport to -1 if
>   this would happen.
> - For GetPortStatus, we range-check wIndex (and by implication
>   rhport) and report an error, but *don't* skip the following code.
>   Add a goto to the error path.
> - At the end of the function, there's one last port_status lookup
>   that's not protected by any range check.


I have patch out for this to fix a syzbot reported problem.

console output: https://syzkaller.appspot.com/x/log.txt?x=126a6f0e400000
kernel config:  https://syzkaller.appspot.com/x/.config?x=531a917630d2a492
dashboard link: https://syzkaller.appspot.com/bug?extid=bccc1fe10b70fadc78d0
compiler:       gcc (GCC) 8.0.1 20180413 (experimental)
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=121caa46400000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=14ed8ab6400000

I was able to reproduce the problem with the C reproducer and fixed it.

Here is fix:
https://patchwork.kernel.org/patch/10628833/

Sudip verified the patch.

thanks,
-- Shuah

usbip: vhci_hcd: Check rhport everywhere in vhci_hub_control()

[Ben Hutchings]

Commit 5b22f676118f "usbip: vhci_hcd: check rhport before using in
vhci_hub_control()" added some validation of rhport, but left
several problems:

- If VHCI_HC_PORTS < 256, we can get rhport >= VHCI_HC_PORTS which
  is also out of range.  To keep things simple, set rhport to -1 if
  this would happen.
- For GetPortStatus, we range-check wIndex (and by implication
  rhport) and report an error, but *don't* skip the following code.
  Add a goto to the error path.
- At the end of the function, there's one last port_status lookup
  that's not protected by any range check.

Fixes: 5b22f676118f ("usbip: vhci_hcd: check rhport before using in ...")
Cc: stable@vger.kernel.org
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/usb/usbip/vhci_hcd.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/usbip/vhci_hcd.c b/drivers/usb/usbip/vhci_hcd.c
index d11f3f8dad40..e259d3812641 100644
--- a/drivers/usb/usbip/vhci_hcd.c
+++ b/drivers/usb/usbip/vhci_hcd.c
@@ -334,9 +334,12 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	usbip_dbg_vhci_rh("typeReq %x wValue %x wIndex %x\n", typeReq, wValue,
 			  wIndex);
 
-	if (wIndex > VHCI_HC_PORTS)
+	if (wIndex > VHCI_HC_PORTS) {
 		pr_err("invalid port number %d\n", wIndex);
-	rhport = wIndex - 1;
+		rhport = -1;
+	} else {
+		rhport = wIndex - 1;
+	}
 
 	vhci_hcd = hcd_to_vhci_hcd(hcd);
 	vhci = vhci_hcd->vhci;
@@ -414,10 +417,10 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		*(__le32 *) buf = cpu_to_le32(0);
 		break;
 	case GetPortStatus:
-		usbip_dbg_vhci_rh(" GetPortStatus port %x\n", wIndex);
-		if (wIndex < 1) {
-			pr_err("invalid port number %d\n", wIndex);
-			retval = -EPIPE;
+		usbip_dbg_vhci_rh(" GetPortStatus port %x\n", rhport);
+		if (rhport < 0) {
+			pr_err("invalid port number %d\n", rhport);
+			goto error;
 		}
 
 		/* we do not care about resume. */
@@ -618,7 +621,7 @@ static int vhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 
 	spin_unlock_irqrestore(&vhci->lock, flags);
 
-	if ((vhci_hcd->port_status[rhport] & PORT_C_MASK) != 0)
+	if (rhport >= 0 && (vhci_hcd->port_status[rhport] & PORT_C_MASK) != 0)
 		usb_hcd_poll_rh_status(hcd);
 
 	return retval;