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

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

[Minhong He]

Check the return value of register_netdevice_notifier() after a successful
can_proto_register(). If notifier registration fails, unregister the CAN
protocol before returning the error.

Fixes: 8d0caedb7596 ("can: bcm/raw/isotp: use per module netdevice notifier")

Signed-off-by: Minhong He <heminhong@kylinos.cn>
---
 net/can/isotp.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
index 54becaf6898f..65e890cafd7a 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;
+	}
+
+	return 0;
 }
 
 static __exit void isotp_module_exit(void)
-- 
2.25.1

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

[sashiko-bot]

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 <heminhong@kylinos.cn>

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/20260728031038.76524-1-heminhong@kylinos.cn?part=1

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

[Oliver Hartkopp]

Hello Minhong He,

thanks for your improvement - but sashiko-bot is right too.

Currently isotp.c is the only CAN module that misses the error handling 
and the rule of thumb to expose the userspace visibility as a last step.

bcm.c and raw.c are already correct in this matter.

Please send a V2 which also references the bcm.c/raw.c init functions:

"Align isotp_module_init() with the reordering already done for raw.c 
(commit …) and bcm.c (commit …)."

Best regards,
Oliver

On 28.07.26 05:20, sashiko-bot@kernel.org wrote:
> 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 <heminhong@kylinos.cn>
> 
> 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?
>