* [PATCH net] net: hso: check for allocation failure in hso_create_bulk_serial_device() @ 2021-05-12 10:09 Dan Carpenter 2021-05-12 10:19 ` Johan Hovold 0 siblings, 1 reply; 6+ messages in thread From: Dan Carpenter @ 2021-05-12 10:09 UTC (permalink / raw) To: David S. Miller, Denis Joseph Barrow Cc: Jakub Kicinski, Oliver Neukum, Anirudh Rayabharam, Greg Kroah-Hartman, Johan Hovold, Rustam Kovhaev, Zheng Yongjun, Emil Renner Berthing, linux-usb, netdev, kernel-janitors Add a couple checks for if these allocations fail. Fixes: 542f54823614 ("tty: Modem functions for the HSO driver") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/net/usb/hso.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c index 3ef4b2841402..3b2a868d7a72 100644 --- a/drivers/net/usb/hso.c +++ b/drivers/net/usb/hso.c @@ -2618,9 +2618,13 @@ static struct hso_device *hso_create_bulk_serial_device( num_urbs = 2; serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), GFP_KERNEL); + if (!serial->tiocmget) + goto exit; serial->tiocmget->serial_state_notification = kzalloc(sizeof(struct hso_serial_state_notification), GFP_KERNEL); + if (!serial->tiocmget->serial_state_notification) + goto exit; /* it isn't going to break our heart if serial->tiocmget * allocation fails don't bother checking this. */ -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: hso: check for allocation failure in hso_create_bulk_serial_device() 2021-05-12 10:09 [PATCH net] net: hso: check for allocation failure in hso_create_bulk_serial_device() Dan Carpenter @ 2021-05-12 10:19 ` Johan Hovold 2021-05-12 13:12 ` Dan Carpenter 2021-05-14 14:24 ` [PATCH v2 " Dan Carpenter 0 siblings, 2 replies; 6+ messages in thread From: Johan Hovold @ 2021-05-12 10:19 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Denis Joseph Barrow, Jakub Kicinski, Oliver Neukum, Anirudh Rayabharam, Greg Kroah-Hartman, Rustam Kovhaev, Zheng Yongjun, Emil Renner Berthing, linux-usb, netdev, kernel-janitors On Wed, May 12, 2021 at 01:09:04PM +0300, Dan Carpenter wrote: > Add a couple checks for if these allocations fail. > > Fixes: 542f54823614 ("tty: Modem functions for the HSO driver") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/net/usb/hso.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c > index 3ef4b2841402..3b2a868d7a72 100644 > --- a/drivers/net/usb/hso.c > +++ b/drivers/net/usb/hso.c > @@ -2618,9 +2618,13 @@ static struct hso_device *hso_create_bulk_serial_device( > num_urbs = 2; > serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), > GFP_KERNEL); > + if (!serial->tiocmget) > + goto exit; Nice catch; the next assignment would go boom if this ever failed. This appears to have been introduced by af0de1303c4e ("usb: hso: obey DMA rules in tiocmget") > serial->tiocmget->serial_state_notification > = kzalloc(sizeof(struct hso_serial_state_notification), > GFP_KERNEL); > + if (!serial->tiocmget->serial_state_notification) > + goto exit; > /* it isn't going to break our heart if serial->tiocmget > * allocation fails don't bother checking this. > */ You should remove this comment and drop the conditional on the following line as well now, though. Johan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: hso: check for allocation failure in hso_create_bulk_serial_device() 2021-05-12 10:19 ` Johan Hovold @ 2021-05-12 13:12 ` Dan Carpenter 2021-05-14 14:24 ` [PATCH v2 " Dan Carpenter 1 sibling, 0 replies; 6+ messages in thread From: Dan Carpenter @ 2021-05-12 13:12 UTC (permalink / raw) To: Johan Hovold Cc: David S. Miller, Denis Joseph Barrow, Jakub Kicinski, Oliver Neukum, Anirudh Rayabharam, Greg Kroah-Hartman, Rustam Kovhaev, Zheng Yongjun, Emil Renner Berthing, linux-usb, netdev, kernel-janitors On Wed, May 12, 2021 at 12:19:03PM +0200, Johan Hovold wrote: > On Wed, May 12, 2021 at 01:09:04PM +0300, Dan Carpenter wrote: > > Add a couple checks for if these allocations fail. > > > > Fixes: 542f54823614 ("tty: Modem functions for the HSO driver") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/net/usb/hso.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c > > index 3ef4b2841402..3b2a868d7a72 100644 > > --- a/drivers/net/usb/hso.c > > +++ b/drivers/net/usb/hso.c > > @@ -2618,9 +2618,13 @@ static struct hso_device *hso_create_bulk_serial_device( > > num_urbs = 2; > > serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), > > GFP_KERNEL); > > + if (!serial->tiocmget) > > + goto exit; > > Nice catch; the next assignment would go boom if this ever failed. > > This appears to have been introduced by > > af0de1303c4e ("usb: hso: obey DMA rules in tiocmget") > > > serial->tiocmget->serial_state_notification > > = kzalloc(sizeof(struct hso_serial_state_notification), > > GFP_KERNEL); > > + if (!serial->tiocmget->serial_state_notification) > > + goto exit; > > /* it isn't going to break our heart if serial->tiocmget > > * allocation fails don't bother checking this. > > */ > > You should remove this comment and drop the conditional on the following > line as well now, though. Ah, good catch. I'll resend. Thanks! regards, dan carpenter ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 net] net: hso: check for allocation failure in hso_create_bulk_serial_device() 2021-05-12 10:19 ` Johan Hovold 2021-05-12 13:12 ` Dan Carpenter @ 2021-05-14 14:24 ` Dan Carpenter 2021-05-17 8:07 ` Johan Hovold 2021-05-17 21:00 ` patchwork-bot+netdevbpf 1 sibling, 2 replies; 6+ messages in thread From: Dan Carpenter @ 2021-05-14 14:24 UTC (permalink / raw) To: David S. Miller, Johan Hovold Cc: Jakub Kicinski, Oliver Neukum, Anirudh Rayabharam, Johan Hovold, Zheng Yongjun, Geert Uytterhoeven, Emil Renner Berthing, Rustam Kovhaev, linux-usb, netdev, kernel-janitors In current kernels, small allocations never actually fail so this patch shouldn't affect runtime. Originally this error handling code written with the idea that if the "serial->tiocmget" allocation failed, then we would continue operating instead of bailing out early. But in later years we added an unchecked dereference on the next line. serial->tiocmget->serial_state_notification = kzalloc(); ^^^^^^^^^^^^^^^^^^ Since these allocations are never going fail in real life, this is mostly a philosophical debate, but I think bailing out early is the correct behavior that the user would want. And generally it's safer to bail as soon an error happens. Fixes: af0de1303c4e ("usb: hso: obey DMA rules in tiocmget") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: Do more extensive clean up. As Johan pointed out the comments and later NULL checks can be removed. drivers/net/usb/hso.c | 37 ++++++++++++++++++------------------- 1 file changed, 18 insertions(+), 19 deletions(-) diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c index 3ef4b2841402..260f850d69eb 100644 --- a/drivers/net/usb/hso.c +++ b/drivers/net/usb/hso.c @@ -2618,29 +2618,28 @@ static struct hso_device *hso_create_bulk_serial_device( num_urbs = 2; serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), GFP_KERNEL); + if (!serial->tiocmget) + goto exit; serial->tiocmget->serial_state_notification = kzalloc(sizeof(struct hso_serial_state_notification), GFP_KERNEL); - /* it isn't going to break our heart if serial->tiocmget - * allocation fails don't bother checking this. - */ - if (serial->tiocmget && serial->tiocmget->serial_state_notification) { - tiocmget = serial->tiocmget; - tiocmget->endp = hso_get_ep(interface, - USB_ENDPOINT_XFER_INT, - USB_DIR_IN); - if (!tiocmget->endp) { - dev_err(&interface->dev, "Failed to find INT IN ep\n"); - goto exit; - } - - tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); - if (tiocmget->urb) { - mutex_init(&tiocmget->mutex); - init_waitqueue_head(&tiocmget->waitq); - } else - hso_free_tiomget(serial); + if (!serial->tiocmget->serial_state_notification) + goto exit; + tiocmget = serial->tiocmget; + tiocmget->endp = hso_get_ep(interface, + USB_ENDPOINT_XFER_INT, + USB_DIR_IN); + if (!tiocmget->endp) { + dev_err(&interface->dev, "Failed to find INT IN ep\n"); + goto exit; } + + tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); + if (tiocmget->urb) { + mutex_init(&tiocmget->mutex); + init_waitqueue_head(&tiocmget->waitq); + } else + hso_free_tiomget(serial); } else num_urbs = 1; -- 2.30.2 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] net: hso: check for allocation failure in hso_create_bulk_serial_device() 2021-05-14 14:24 ` [PATCH v2 " Dan Carpenter @ 2021-05-17 8:07 ` Johan Hovold 2021-05-17 21:00 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 6+ messages in thread From: Johan Hovold @ 2021-05-17 8:07 UTC (permalink / raw) To: Dan Carpenter Cc: David S. Miller, Jakub Kicinski, Oliver Neukum, Anirudh Rayabharam, Zheng Yongjun, Geert Uytterhoeven, Emil Renner Berthing, Rustam Kovhaev, linux-usb, netdev, kernel-janitors On Fri, May 14, 2021 at 05:24:48PM +0300, Dan Carpenter wrote: > In current kernels, small allocations never actually fail so this > patch shouldn't affect runtime. > > Originally this error handling code written with the idea that if > the "serial->tiocmget" allocation failed, then we would continue > operating instead of bailing out early. But in later years we added > an unchecked dereference on the next line. > > serial->tiocmget->serial_state_notification = kzalloc(); > ^^^^^^^^^^^^^^^^^^ > > Since these allocations are never going fail in real life, this is > mostly a philosophical debate, but I think bailing out early is the > correct behavior that the user would want. And generally it's safer to > bail as soon an error happens. > > Fixes: af0de1303c4e ("usb: hso: obey DMA rules in tiocmget") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > v2: Do more extensive clean up. As Johan pointed out the comments and > later NULL checks can be removed. > > drivers/net/usb/hso.c | 37 ++++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 19 deletions(-) > > diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c > index 3ef4b2841402..260f850d69eb 100644 > --- a/drivers/net/usb/hso.c > +++ b/drivers/net/usb/hso.c > @@ -2618,29 +2618,28 @@ static struct hso_device *hso_create_bulk_serial_device( > num_urbs = 2; > serial->tiocmget = kzalloc(sizeof(struct hso_tiocmget), > GFP_KERNEL); > + if (!serial->tiocmget) > + goto exit; > serial->tiocmget->serial_state_notification > = kzalloc(sizeof(struct hso_serial_state_notification), > GFP_KERNEL); > - /* it isn't going to break our heart if serial->tiocmget > - * allocation fails don't bother checking this. > - */ > - if (serial->tiocmget && serial->tiocmget->serial_state_notification) { > - tiocmget = serial->tiocmget; > - tiocmget->endp = hso_get_ep(interface, > - USB_ENDPOINT_XFER_INT, > - USB_DIR_IN); > - if (!tiocmget->endp) { > - dev_err(&interface->dev, "Failed to find INT IN ep\n"); > - goto exit; > - } > - > - tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); > - if (tiocmget->urb) { > - mutex_init(&tiocmget->mutex); > - init_waitqueue_head(&tiocmget->waitq); > - } else > - hso_free_tiomget(serial); > + if (!serial->tiocmget->serial_state_notification) > + goto exit; > + tiocmget = serial->tiocmget; > + tiocmget->endp = hso_get_ep(interface, > + USB_ENDPOINT_XFER_INT, > + USB_DIR_IN); > + if (!tiocmget->endp) { > + dev_err(&interface->dev, "Failed to find INT IN ep\n"); > + goto exit; > } > + > + tiocmget->urb = usb_alloc_urb(0, GFP_KERNEL); > + if (tiocmget->urb) { > + mutex_init(&tiocmget->mutex); > + init_waitqueue_head(&tiocmget->waitq); > + } else > + hso_free_tiomget(serial); This should probably be changed to bail out on allocation errors as well now but that can be done as a follow-up. Either way: Reviewed-by: Johan Hovold <johan@kernel.org> > } > else > num_urbs = 1; Johan ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 net] net: hso: check for allocation failure in hso_create_bulk_serial_device() 2021-05-14 14:24 ` [PATCH v2 " Dan Carpenter 2021-05-17 8:07 ` Johan Hovold @ 2021-05-17 21:00 ` patchwork-bot+netdevbpf 1 sibling, 0 replies; 6+ messages in thread From: patchwork-bot+netdevbpf @ 2021-05-17 21:00 UTC (permalink / raw) To: Dan Carpenter Cc: davem, johan, kuba, oneukum, mail, zhengyongjun3, geert, kernel, rkovhaev, linux-usb, netdev, kernel-janitors Hello: This patch was applied to netdev/net.git (refs/heads/master): On Fri, 14 May 2021 17:24:48 +0300 you wrote: > In current kernels, small allocations never actually fail so this > patch shouldn't affect runtime. > > Originally this error handling code written with the idea that if > the "serial->tiocmget" allocation failed, then we would continue > operating instead of bailing out early. But in later years we added > an unchecked dereference on the next line. > > [...] Here is the summary with links: - [v2,net] net: hso: check for allocation failure in hso_create_bulk_serial_device() https://git.kernel.org/netdev/net/c/31db0dbd7244 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-05-17 21:00 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-12 10:09 [PATCH net] net: hso: check for allocation failure in hso_create_bulk_serial_device() Dan Carpenter 2021-05-12 10:19 ` Johan Hovold 2021-05-12 13:12 ` Dan Carpenter 2021-05-14 14:24 ` [PATCH v2 " Dan Carpenter 2021-05-17 8:07 ` Johan Hovold 2021-05-17 21:00 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).