[PATCH 1/2] usb: hso: fix error handling code of hso_create_net_device

The current error handling code of hso_create_net_device is
hso_free_net_device, no matter which errors lead to. For example,
WARNING in hso_free_net_device [1].

Fix this by refactoring the error handling code of
hso_create_net_device by handling different errors by different code.

[1] https://syzkaller.appspot.com/bug?id=66eff8d49af1b28370ad342787413e35bbe76efe

Reported-by: syzbot+44d53c7255bb1aea22d2@syzkaller.appspotmail.com
Fixes: 5fcfb6d0bfcd ("hso: fix bailout in error case of probe")
Signed-off-by: Dongliang Mu <redacted>
---
 drivers/net/usb/hso.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 54ef8492ca01..90fa4d9fa119 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c

@@ -2495,7 +2495,9 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 			   hso_net_init);
 	if (!net) {
 		dev_err(&interface->dev, "Unable to create ethernet device\n");
-		goto exit;
+		kfree(hso_dev);
+	usb_free_urb(hso_net->mux_bulk_tx_urb);
+		return NULL;
 	}
 
 	hso_net = netdev_priv(net);

@@ -2508,13 +2510,13 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 				      USB_DIR_IN);
 	if (!hso_net->in_endp) {
 		dev_err(&interface->dev, "Can't find BULK IN endpoint\n");
-		goto exit;
+		goto err_get_ep;
 	}
 	hso_net->out_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
 				       USB_DIR_OUT);
 	if (!hso_net->out_endp) {
 		dev_err(&interface->dev, "Can't find BULK OUT endpoint\n");
-		goto exit;
+		goto err_get_ep;
 	}
 	SET_NETDEV_DEV(net, &interface->dev);
 	SET_NETDEV_DEVTYPE(net, &hso_type);

@@ -2523,18 +2525,18 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
 		hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
 		if (!hso_net->mux_bulk_rx_urb_pool[i])
-			goto exit;
+			goto err_mux_bulk_rx;
 		hso_net->mux_bulk_rx_buf_pool[i] = kzalloc(MUX_BULK_RX_BUF_SIZE,
 							   GFP_KERNEL);
 		if (!hso_net->mux_bulk_rx_buf_pool[i])
-			goto exit;
+			goto err_mux_bulk_rx;
 	}
 	hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_urb)
-		goto exit;
+		goto err_mux_bulk_tx;
 	hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_buf)
-		goto exit;
+		goto err_mux_bulk_tx;
 
 	add_net_device(hso_dev);
 

@@ -2542,7 +2544,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	result = register_netdev(net);
 	if (result) {
 		dev_err(&interface->dev, "Failed to register device\n");
-		goto exit;
+		goto err_register;
 	}
 
 	hso_log_port(hso_dev);

@@ -2550,8 +2552,23 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
 	hso_create_rfkill(hso_dev, interface);
 
 	return hso_dev;
-exit:
-	hso_free_net_device(hso_dev, true);
+
+err_register:
+	unregister_netdev(net);
+	remove_net_device(hso_dev);
+err_mux_bulk_tx:
+	kfree(hso_net->mux_bulk_tx_buf);
+	hso_net->mux_bulk_tx_buf = NULL;
+	usb_free_urb(hso_net->mux_bulk_tx_urb);
+err_mux_bulk_rx:
+	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
+		usb_free_urb(hso_net->mux_bulk_rx_urb_pool[i]);
+		kfree(hso_net->mux_bulk_rx_buf_pool[i]);
+		hso_net->mux_bulk_rx_buf_pool[i] = NULL;
+	}
+err_get_ep:
+	free_netdev(net);
+	kfree(hso_dev);
 	return NULL;
 }
 

-- 
2.25.1

There are two invocation sites of hso_free_net_device. After
refactoring hso_create_net_device, this parameter is useless.
Remove the bailout in the hso_free_net_device and change the invocation
sites of this function

Signed-off-by: Dongliang Mu <redacted>
---
 drivers/net/usb/hso.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 90fa4d9fa119..91cace7aa657 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c

@@ -2353,7 +2353,7 @@ static int remove_net_device(struct hso_device *hso_dev)
 }
 
 /* Frees our network device */
-static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
+static void hso_free_net_device(struct hso_device *hso_dev)
 {
 	int i;
 	struct hso_net *hso_net = dev2net(hso_dev);

@@ -2376,7 +2376,7 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout)
 	kfree(hso_net->mux_bulk_tx_buf);
 	hso_net->mux_bulk_tx_buf = NULL;
 
-	if (hso_net->net && !bailout)
+	if (hso_net->net)
 		free_netdev(hso_net->net);
 
 	kfree(hso_dev);

@@ -3137,7 +3137,7 @@ static void hso_free_interface(struct usb_interface *interface)
 				rfkill_unregister(rfk);
 				rfkill_destroy(rfk);
 			}
-			hso_free_net_device(network_table[i], false);
+			hso_free_net_device(network_table[i]);
 		}
 	}
 }

-- 
2.25.1

On Wed, Jul 14, 2021 at 3:16 PM Dongliang Mu [off-list ref] wrote:

The current error handling code of hso_create_net_device is
hso_free_net_device, no matter which errors lead to. For example,
WARNING in hso_free_net_device [1].

Although there is already a patch for the above bug report, I don't
think it cannot handle all kinds of errors caused in
hso_create_net_device.

So refactoring the error handling code is the only way to fix this issue.

[1] https://syzkaller.appspot.com/text?tag=Patch&x=1188fcc6600000

On Wed, Jul 14, 2021 at 3:36 PM Dan Carpenter [off-list ref] wrote:

On Wed, Jul 14, 2021 at 03:15:32PM +0800, Dongliang Mu wrote:

quoted

The current error handling code of hso_create_net_device is
hso_free_net_device, no matter which errors lead to. For example,
WARNING in hso_free_net_device [1].

Fix this by refactoring the error handling code of
hso_create_net_device by handling different errors by different code.

[1] https://syzkaller.appspot.com/bug?id=66eff8d49af1b28370ad342787413e35bbe76efe

Reported-by: syzbot+44d53c7255bb1aea22d2@syzkaller.appspotmail.com
Fixes: 5fcfb6d0bfcd ("hso: fix bailout in error case of probe")
Signed-off-by: Dongliang Mu <redacted>
---
 drivers/net/usb/hso.c | 37 +++++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c
index 54ef8492ca01..90fa4d9fa119 100644
--- a/drivers/net/usb/hso.c
+++ b/drivers/net/usb/hso.c

@@ -2495,7 +2495,9 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
                         hso_net_init);
      if (!net) {
              dev_err(&interface->dev, "Unable to create ethernet device\n");
-             goto exit;
+             kfree(hso_dev);
+     usb_free_urb(hso_net->mux_bulk_tx_urb);

Obviously this wasn't intentional.

This is a copy-paste error. I am sorry. :(

quoted

+             return NULL;

But use gotos here.

OK, I will change it in version v2.

quoted

      }

      hso_net = netdev_priv(net);

@@ -2508,13 +2510,13 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
                                    USB_DIR_IN);
      if (!hso_net->in_endp) {
              dev_err(&interface->dev, "Can't find BULK IN endpoint\n");
-             goto exit;
+             goto err_get_ep;

This is Come From naming style where it says what failed on the line
before.  It's not helpful because we can see what failed.  What we need
to know is what the goto does.

Use Free the Last thing style.  Where you just keep track of the most
recent successful allocation and free it.  That way you don't free
things which aren't allocated, you don't double free things, you don't
dereference uninitialized variables or error points.  Plus it's a very
simple system where when you're reading code you just have to remember
the last thing that was allocated.  Every function must clean up after
itself.  Every allocation function needs a free function.  The goto
names say the variable that is freed.

That's a good rule to follow. Will change the patch following this rule.

                goto free_net;

quoted

      }
      hso_net->out_endp = hso_get_ep(interface, USB_ENDPOINT_XFER_BULK,
                                     USB_DIR_OUT);
      if (!hso_net->out_endp) {
              dev_err(&interface->dev, "Can't find BULK OUT endpoint\n");
-             goto exit;
+             goto err_get_ep;
      }
      SET_NETDEV_DEV(net, &interface->dev);
      SET_NETDEV_DEVTYPE(net, &hso_type);

@@ -2523,18 +2525,18 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
      for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
              hso_net->mux_bulk_rx_urb_pool[i] = usb_alloc_urb(0, GFP_KERNEL);
              if (!hso_net->mux_bulk_rx_urb_pool[i])
-                     goto exit;
+                     goto err_mux_bulk_rx;
              hso_net->mux_bulk_rx_buf_pool[i] = kzalloc(MUX_BULK_RX_BUF_SIZE,
                                                         GFP_KERNEL);
              if (!hso_net->mux_bulk_rx_buf_pool[i])
-                     goto exit;
+                     goto err_mux_bulk_rx;

In a loop then how Free the last thing style works is that you free
that partial allocation before the goto.  And then do a

        while (--i >= 0) {
                free_c();
                free_b();
                free_a();
        }

You have told me this rule. But I think the handling of this loop does
not need to be such complicated.

But in this case your code is fine and simple enough.  No need to be
dogmatic about style so long as the functions are small.

Thanks.

quoted

      }
      hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
      if (!hso_net->mux_bulk_tx_urb)
-             goto exit;
+             goto err_mux_bulk_tx;
      hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
      if (!hso_net->mux_bulk_tx_buf)
-             goto exit;
+             goto err_mux_bulk_tx;

These gotos are freeing things which haven't been allocated.  Which is
harmless in this case but puzzling.

I will revise this label in the v2 version.

quoted

      add_net_device(hso_dev);

@@ -2542,7 +2544,7 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
      result = register_netdev(net);
      if (result) {
              dev_err(&interface->dev, "Failed to register device\n");
-             goto exit;
+             goto err_register;

In this case register failed and calling unregister_netdev() will lead
to WARN_ON(1) and a stack trace.

I will revise this label in the v2 version.

quoted

      }

      hso_log_port(hso_dev);

@@ -2550,8 +2552,23 @@ static struct hso_device *hso_create_net_device(struct usb_interface *interface,
      hso_create_rfkill(hso_dev, interface);

      return hso_dev;
-exit:
-     hso_free_net_device(hso_dev, true);
+
+err_register:
+     unregister_netdev(net);
+     remove_net_device(hso_dev);
+err_mux_bulk_tx:
+     kfree(hso_net->mux_bulk_tx_buf);
+     hso_net->mux_bulk_tx_buf = NULL;

No need for this.

quoted

+     usb_free_urb(hso_net->mux_bulk_tx_urb);
+err_mux_bulk_rx:
+     for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
+             usb_free_urb(hso_net->mux_bulk_rx_urb_pool[i]);
+             kfree(hso_net->mux_bulk_rx_buf_pool[i]);
+             hso_net->mux_bulk_rx_buf_pool[i] = NULL;

No need.  This memory is just going to be freed.

quoted

+     }
+err_get_ep:
+     free_netdev(net);
+     kfree(hso_dev);
      return NULL;
 }

regards,
dan carpenter

On Wed, Jul 14, 2021 at 03:15:32PM +0800, Dongliang Mu wrote:

Obviously this wasn't intentional.

+		return NULL;

But use gotos here.

This is Come From naming style where it says what failed on the line
before.  It's not helpful because we can see what failed.  What we need
to know is what the goto does.

Use Free the Last thing style.  Where you just keep track of the most
recent successful allocation and free it.  That way you don't free
things which aren't allocated, you don't double free things, you don't
dereference uninitialized variables or error points.  Plus it's a very
simple system where when you're reading code you just have to remember
the last thing that was allocated.  Every function must clean up after
itself.  Every allocation function needs a free function.  The goto
names say the variable that is freed.

		goto free_net;

In a loop then how Free the last thing style works is that you free
that partial allocation before the goto.  And then do a

	while (--i >= 0) {
		free_c();
		free_b();
		free_a();
	}

But in this case your code is fine and simple enough.  No need to be
dogmatic about style so long as the functions are small.

 	}
 	hso_net->mux_bulk_tx_urb = usb_alloc_urb(0, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_urb)
-		goto exit;
+		goto err_mux_bulk_tx;
 	hso_net->mux_bulk_tx_buf = kzalloc(MUX_BULK_TX_BUF_SIZE, GFP_KERNEL);
 	if (!hso_net->mux_bulk_tx_buf)
-		goto exit;
+		goto err_mux_bulk_tx;

These gotos are freeing things which haven't been allocated.  Which is
harmless in this case but puzzling.

In this case register failed and calling unregister_netdev() will lead
to WARN_ON(1) and a stack trace.

No need for this.

+	usb_free_urb(hso_net->mux_bulk_tx_urb);
+err_mux_bulk_rx:
+	for (i = 0; i < MUX_BULK_RX_BUF_COUNT; i++) {
+		usb_free_urb(hso_net->mux_bulk_rx_urb_pool[i]);
+		kfree(hso_net->mux_bulk_rx_buf_pool[i]);
+		hso_net->mux_bulk_rx_buf_pool[i] = NULL;

No need.  This memory is just going to be freed.

+	}
+err_get_ep:
+	free_netdev(net);
+	kfree(hso_dev);
 	return NULL;
 }

regards,
dan carpenter