| Message ID | 20181011053057.GA3375@decadent.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usbip: vhci_hcd: Check rhport everywhere in vhci_hub_control() | expand |
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
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;
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(-)