| Message ID | 20230811230723.59234-1-CFSworks@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: usbip: fix use-after-free race | expand |
On 8/11/23 17:07, Sam Edwards wrote: > stub_recv_cmd_submit() allocates a `priv` structure, which is freed by > the TX thread after all URBs in the `priv` complete and are handled. > This means that stub_recv_cmd_submit() effectively loses ownership of > `priv` once the final URB is submitted: in the worst case, the RX > thread will be preempted before usb_submit_urb() returns, and the TX > thread will do all handling and cleanup before the RX thread resumes. > How did you find this problem? Please add the details in the change log and also please describe the fix in detail. This patch changes for loop from looping on priv->num_urbs to num_urbs. Change looks okay to me. I do want to know how you found the problem. > We don't lose ownership if usb_submit_urb() returns an error value, > since that means it won't eventually call stub_complete(), so the > error-handling `usbip_dump_urb(priv->urbs[i])` is still safe. > Did you happen to track down where this urb gets free'd after usbip_dump_urb(priv->urbs[i]) > Signed-off-by: Sam Edwards <CFSworks@gmail.com> > --- > drivers/usb/usbip/stub_rx.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c > index fc01b31bbb87..dba9a64830e6 100644 > --- a/drivers/usb/usbip/stub_rx.c > +++ b/drivers/usb/usbip/stub_rx.c > @@ -592,8 +592,11 @@ static void stub_recv_cmd_submit(struct stub_device *sdev, > if (usbip_recv_iso(ud, priv->urbs[0]) < 0) > return; > > - /* urb is now ready to submit */ > - for (i = 0; i < priv->num_urbs; i++) { > + /* > + * URB(s) are now ready to submit. > + * Note: priv is freed after the last URB is (successfully) submitted. > + */ > + for (i = 0; i < num_urbs; i++) { > ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL); > > if (ret == 0) thanks, -- Shuah
diff --git a/drivers/usb/usbip/stub_rx.c b/drivers/usb/usbip/stub_rx.c index fc01b31bbb87..dba9a64830e6 100644 --- a/drivers/usb/usbip/stub_rx.c +++ b/drivers/usb/usbip/stub_rx.c @@ -592,8 +592,11 @@ static void stub_recv_cmd_submit(struct stub_device *sdev, if (usbip_recv_iso(ud, priv->urbs[0]) < 0) return; - /* urb is now ready to submit */ - for (i = 0; i < priv->num_urbs; i++) { + /* + * URB(s) are now ready to submit. + * Note: priv is freed after the last URB is (successfully) submitted. + */ + for (i = 0; i < num_urbs; i++) { ret = usb_submit_urb(priv->urbs[i], GFP_KERNEL); if (ret == 0)
stub_recv_cmd_submit() allocates a `priv` structure, which is freed by the TX thread after all URBs in the `priv` complete and are handled. This means that stub_recv_cmd_submit() effectively loses ownership of `priv` once the final URB is submitted: in the worst case, the RX thread will be preempted before usb_submit_urb() returns, and the TX thread will do all handling and cleanup before the RX thread resumes. We don't lose ownership if usb_submit_urb() returns an error value, since that means it won't eventually call stub_complete(), so the error-handling `usbip_dump_urb(priv->urbs[i])` is still safe. Signed-off-by: Sam Edwards <CFSworks@gmail.com> --- drivers/usb/usbip/stub_rx.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)