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