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

[Dan Carpenter]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 5fcfb6d0bfcd ("hso: fix bailout in error case of probe")
> Signed-off-by: Dongliang Mu <mudongliangabcd@xxxxxxxxx>
> ---
>  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