Re: [PATCH net] can: isotp: check register_netdevice_notifier() error in module init

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Registering the CAN protocol before completing all fallible initialization steps creates a race window leading to a Use-After-Free crash if initialization fails.
--

--- Patch [1]: [PATCH net] can: isotp: check register_netdevice_notifier() error in module init ---
commit 5f2f7a58360e9a2c18b5d6a9a724f468f79227d1
Author: Minhong He <[email protected]>

can: isotp: check register_netdevice_notifier() error in module init

This commit adds a check for the return value of register_netdevice_notifier()
in the isotp module initialization. If the notifier registration fails, it
now unregisters the CAN protocol and returns the error.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..65e890cafd7a9 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -1908,12 +1908,18 @@ static __init int isotp_module_init(void)
>  	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
>  
>  	err = can_proto_register(&isotp_can_proto);
> -	if (err < 0)
> +	if (err < 0) {
>  		pr_err("can: registration of isotp protocol failed %pe\n", ERR_PTR(err));
> -	else
> -		register_netdevice_notifier(&canisotp_notifier);
> +		return err;
> +	}
>  
> -	return err;
> +	err = register_netdevice_notifier(&canisotp_notifier);
> +	if (err) {
> +		can_proto_unregister(&isotp_can_proto);
> +		return err;
> +	}

[Severity: High]
Does this error path introduce a use-after-free risk if userspace creates a
socket before the module initialization completes?

If can_proto_register() succeeds, the protocol is immediately exposed to
userspace. If a concurrent thread creates an ISOTP socket before
register_netdevice_notifier() fails, the socket will hold pointers to the
module's protocol operations.

When the module initialization aborts and returns an error, the kernel will
free the module memory. While can_proto_unregister() removes the protocol
from the registry, it does not revoke already-created sockets. As a result,
any subsequent operations on the existing socket will dereference freed
module memory.

Would it be safer to move the fallible register_netdevice_notifier() before
can_proto_register() to avoid exposing the protocol before initialization
is guaranteed to succeed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1