| Message ID | 20191021142414.27164-1-suwan.kim027@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usbip: Fix free of unallocated memory in vhci tx | expand |
On Mon, 21 Oct 2019, Suwan Kim wrote: > iso_buffer should be set to NULL after use and free in the while loop. > In the case of isochronous URB in the while loop, iso_buffer is > allocated and after sending it to server, buffer is deallocated. And > then, if the next URB in the while loop is not a isochronous pipe, > iso_buffer still holds the previously deallocated buffer address and > kfree tries to free wrong buffer address. > > Fixes: ea44d190764b (“usbip: Implement SG support to vhci-hcd and stub driver”) > Reported-by: kbuild test robot <lkp@intel.com> > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > --- > drivers/usb/usbip/vhci_tx.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c > index c3803785f6ef..b290e810d11b 100644 > --- a/drivers/usb/usbip/vhci_tx.c > +++ b/drivers/usb/usbip/vhci_tx.c > @@ -73,6 +73,7 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev) > memset(&pdu_header, 0, sizeof(pdu_header)); > memset(&msg, 0, sizeof(msg)); > memset(&iov, 0, sizeof(iov)); > + iso_buffer = NULL; Somehow I would have put it after the kfree, since the kfree makes the value invalid. iso_buffer is already initialized to NULL for the first iteration. If you want to put the setting to NULL at the top of the loop, maybe the = NULL in the first line should be removed. julia > > usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n", > priv->seqnum); > -- > 2.21.0 > >
On 10/21/19 9:08 AM, Julia Lawall wrote: > > > On Mon, 21 Oct 2019, Suwan Kim wrote: > >> iso_buffer should be set to NULL after use and free in the while loop. >> In the case of isochronous URB in the while loop, iso_buffer is >> allocated and after sending it to server, buffer is deallocated. And >> then, if the next URB in the while loop is not a isochronous pipe, >> iso_buffer still holds the previously deallocated buffer address and >> kfree tries to free wrong buffer address. >> >> Fixes: ea44d190764b (“usbip: Implement SG support to vhci-hcd and stub driver”) >> Reported-by: kbuild test robot <lkp@intel.com> >> Reported-by: Julia Lawall <julia.lawall@lip6.fr> >> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> >> --- >> drivers/usb/usbip/vhci_tx.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c >> index c3803785f6ef..b290e810d11b 100644 >> --- a/drivers/usb/usbip/vhci_tx.c >> +++ b/drivers/usb/usbip/vhci_tx.c >> @@ -73,6 +73,7 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev) >> memset(&pdu_header, 0, sizeof(pdu_header)); >> memset(&msg, 0, sizeof(msg)); >> memset(&iov, 0, sizeof(iov)); >> + iso_buffer = NULL; > > Somehow I would have put it after the kfree, since the kfree makes the > value invalid. iso_buffer is already initialized to NULL for the first > iteration. If you want to put the setting to NULL at the top of the loop, > maybe the = NULL in the first line should be removed. > It makes sense to clear it after kfree() on line 150. This kfree() and clearing iso_buffer are necessary only for PIPE_ISOCHRONOUS case. Please add a comment above that this is for isoc case. thanks, -- Shuah
On Mon, Oct 21, 2019 at 01:28:54PM -0600, shuah wrote: > On 10/21/19 9:08 AM, Julia Lawall wrote: > > > > > > On Mon, 21 Oct 2019, Suwan Kim wrote: > > > > > iso_buffer should be set to NULL after use and free in the while loop. > > > In the case of isochronous URB in the while loop, iso_buffer is > > > allocated and after sending it to server, buffer is deallocated. And > > > then, if the next URB in the while loop is not a isochronous pipe, > > > iso_buffer still holds the previously deallocated buffer address and > > > kfree tries to free wrong buffer address. > > > > > > Fixes: ea44d190764b (“usbip: Implement SG support to vhci-hcd and stub driver”) > > > Reported-by: kbuild test robot <lkp@intel.com> > > > Reported-by: Julia Lawall <julia.lawall@lip6.fr> > > > Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> > > > --- > > > drivers/usb/usbip/vhci_tx.c | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c > > > index c3803785f6ef..b290e810d11b 100644 > > > --- a/drivers/usb/usbip/vhci_tx.c > > > +++ b/drivers/usb/usbip/vhci_tx.c > > > @@ -73,6 +73,7 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev) > > > memset(&pdu_header, 0, sizeof(pdu_header)); > > > memset(&msg, 0, sizeof(msg)); > > > memset(&iov, 0, sizeof(iov)); > > > + iso_buffer = NULL; > > > > Somehow I would have put it after the kfree, since the kfree makes the > > value invalid. iso_buffer is already initialized to NULL for the first > > iteration. If you want to put the setting to NULL at the top of the loop, > > maybe the = NULL in the first line should be removed. > > > > It makes sense to clear it after kfree() on line 150. > > This kfree() and clearing iso_buffer are necessary only for > PIPE_ISOCHRONOUS case. > > Please add a comment above that this is for isoc case. Ok. I will send v2. Regards Suwan Kim
diff --git a/drivers/usb/usbip/vhci_tx.c b/drivers/usb/usbip/vhci_tx.c index c3803785f6ef..b290e810d11b 100644 --- a/drivers/usb/usbip/vhci_tx.c +++ b/drivers/usb/usbip/vhci_tx.c @@ -73,6 +73,7 @@ static int vhci_send_cmd_submit(struct vhci_device *vdev) memset(&pdu_header, 0, sizeof(pdu_header)); memset(&msg, 0, sizeof(msg)); memset(&iov, 0, sizeof(iov)); + iso_buffer = NULL; usbip_dbg_vhci_tx("setup txdata urb seqnum %lu\n", priv->seqnum);
iso_buffer should be set to NULL after use and free in the while loop. In the case of isochronous URB in the while loop, iso_buffer is allocated and after sending it to server, buffer is deallocated. And then, if the next URB in the while loop is not a isochronous pipe, iso_buffer still holds the previously deallocated buffer address and kfree tries to free wrong buffer address. Fixes: ea44d190764b (“usbip: Implement SG support to vhci-hcd and stub driver”) Reported-by: kbuild test robot <lkp@intel.com> Reported-by: Julia Lawall <julia.lawall@lip6.fr> Signed-off-by: Suwan Kim <suwan.kim027@gmail.com> --- drivers/usb/usbip/vhci_tx.c | 1 + 1 file changed, 1 insertion(+)