Re: [PATCH v3 2/4] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: gs_usb: ERR_PTR dereference in gs_usb_probe() error cleanup
--

commit 2720d49a596fa1eb1aed7686479119245dbc2334
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

This patch moves register_candev() and unregister_candev() out of
gs_make_candev() and gs_destroy_candev() into gs_usb_probe() and
gs_usb_disconnect() so that all CAN channel structures and parent fields
are fully initialized before registering the netdevs.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 15f28833f2deb..cbe3750bb68ad 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -1593,14 +1582,7 @@ static int gs_usb_probe(struct usb_interface *intf,
>  			/* save error code to return later */
>  			rc = PTR_ERR(parent->canch[i]);
>  
> -			/* on failure destroy previously created candevs */
> -			icount = i;
> -			for (i = 0; i < icount; i++)
> -				gs_destroy_candev(parent->canch[i]);
> -
> -			usb_kill_anchored_urbs(&parent->rx_submitted);
> -			kfree(parent);
> -			return rc;
> +			goto out_gs_destroy_candev;
>  		}
>  		parent->canch[i]->parent = parent;
>  
> @@ -1621,7 +1603,37 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
>  	}
>  
> +	for (i = 0; i < icount; i++) {
> +		struct gs_can *dev = parent->canch[i];
> +
> +		rc = register_candev(dev->netdev);
> +		if (rc) {
> +			const unsigned int cur = i;
> +
> +			dev_err(&intf->dev,
> +				"Couldn't register candev for channel %d (%pe)\n",
> +				i, ERR_PTR(rc));
> +
> +			for (i = 0; i < cur; i++)
> +				unregister_candev(parent->canch[i]->netdev);
> +
> +			goto out_gs_destroy_candev;
> +		}
> +	}
> +
>  	return 0;
> +
> +out_gs_destroy_candev:
> +	for (i = 0; i < parent->channel_cnt; i++) {
> +		if (!parent->canch[i])
> +			continue;
> +
> +		gs_destroy_candev(parent->canch[i]);
> +	}

[Severity: High]
Can this dereference an error pointer in gs_destroy_candev() if
gs_make_candev() fails during gs_usb_probe() in drivers/net/can/usb/gs_usb.c?

When gs_make_candev() fails (for example, if usb_control_msg_recv() for
GS_USB_BREQ_BT_CONST or GS_USB_BREQ_BT_CONST_EXT fails, or alloc_candev()
returns NULL), it returns an ERR_PTR() value which gs_usb_probe() stores in
parent->canch[i]:

	parent->canch[i] = gs_make_candev(i, intf, &dconf);
	if (IS_ERR_OR_NULL(parent->canch[i])) {
		/* save error code to return later */
		rc = PTR_ERR(parent->canch[i]);

		goto out_gs_destroy_candev;
	}

In the out_gs_destroy_candev cleanup loop, the check only tests
!parent->canch[i]. Because the failed slot holds a non-NULL ERR_PTR(), it is
passed to gs_destroy_candev(), which unconditionally dereferences dev via
can_rx_offload_del(&dev->offload) and free_candev(dev->netdev).

Should the cleanup loop check IS_ERR_OR_NULL(parent->canch[i]) instead of
!parent->canch[i]?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-gs_usb-fixes-v3-0-3a68f1f62813@pengutronix.de?part=2