Re: [PATCH v2 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: use-after-free and leak in gs_usb_probe() error cleanup
--

commit 28ced4be5f6d6693ec0ba1f04a6e081bef998aec
Author: Marc Kleine-Budde <mkl@pengutronix.de>

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

This patch moves register_candev() out of gs_make_candev() into a second loop
in gs_usb_probe() so that all CAN interfaces are completely initialized before
registration.

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

[ ... ]

> @@ -1621,7 +1612,36 @@ 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++)
> +				gs_destroy_candev(dev);
> +
> +			for (; i < parent->channel_cnt; i++) {
> +				can_rx_offload_del(&dev->offload);
> +				free_candev(dev->netdev);
> +			}

[Severity: High]
In gs_usb_probe() in drivers/net/can/usb/gs_usb.c, dev is initialized to
parent->canch[cur] where register_candev() failed. Should both error cleanup
loops index parent->canch[i] instead of using dev?

As written, if register_candev() fails on channel cur, both loops repeatedly
operate on parent->canch[cur]. When cur > 0, the first loop calls
gs_destroy_candev() on the unregistered parent->canch[cur] cur times, leaving
parent->canch[0 .. cur - 1] registered with dangling parent pointers after
kfree(parent). The second loop then calls can_rx_offload_del() and
free_candev() on the same dev pointer for each remaining channel, causing a
use-after-free and double-free on dev->netdev while leaking the remaining
parent->canch[cur + 1 .. parent->channel_cnt - 1] net_device allocations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025-gs_usb-fixes-v2-0-535d45097f72@pengutronix.de?part=2