| Message ID | 20210714071547.656587-1-mudongliangabcd@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] usb: hso: fix error handling code of hso_create_net_device | expand |
On Wed, Jul 14, 2021 at 3:16 PM Dongliang Mu <mudongliangabcd@gmail.com> 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 > > 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 <mudongliangabcd@gmail.com> > --- > 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 >
On Wed, Jul 14, 2021 at 03:15:32PM +0800, Dongliang Mu 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]. > > 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 <mudongliangabcd@gmail.com> > --- > 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. > + return NULL; But use gotos here. > } > > 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. goto free_net; > } > 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(); } 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. > > 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. > } > > 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. > + 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
On Wed, Jul 14, 2021 at 3:36 PM Dan Carpenter <dan.carpenter@oracle.com> wrote: > > On Wed, Jul 14, 2021 at 03:15:32PM +0800, Dongliang Mu 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]. > > > > 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 <mudongliangabcd@gmail.com> > > --- > > 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. :( > > > + return NULL; > > But use gotos here. OK, I will change it in version v2. > > > } > > > > 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; > > > } > > 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. > > > } > > 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. > > > > > 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. > > } > > > > 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. > > > + 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
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; }
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 <mudongliangabcd@gmail.com> --- drivers/net/usb/hso.c | 37 +++++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-)