[PATCH net] net: hso: check for allocation failure in hso_create_bulk_serial_device()

Add a couple checks for if these allocations fail.

Fixes: 542f54823614 ("tty: Modem functions for the HSO driver")
Signed-off-by: Dan Carpenter <redacted>
---
 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

On Wed, May 12, 2021 at 01:09:04PM +0300, Dan Carpenter wrote:

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")

You should remove this comment and drop the conditional on the following
line as well now, though.

Johan

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:

quoted

Add a couple checks for if these allocations fail.

Fixes: 542f54823614 ("tty: Modem functions for the HSO driver")
Signed-off-by: Dan Carpenter <redacted>
---
 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")

quoted

 		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

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 <redacted>
---
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

On Fri, May 14, 2021 at 05:24:48PM +0300, Dan Carpenter wrote:

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

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