[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?
> 

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

[Minhong He]

Register the netdevice notifier before can_proto_register() and check the
return value. If protocol registration fails, unregister the notifier
before returning the error.

Align isotp_module_init() with the reordering already done for raw.c
(commit c28b3bffe49e ("can: raw: process optimization in raw_init()")) and
bcm.c (commit edd1a7e42f1d ("can: bcm: registration process optimization
in bcm_module_init()")).

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

Signed-off-by: Minhong He <heminhong@kylinos.cn>
---
v2:
- Register the notifier before can_proto_register(), matching raw/bcm
- Reference the raw.c and bcm.c reordering commits as requested
v1: https://lore.kernel.org/all/20260728031038.76524-1-heminhong@kylinos.cn/

 net/can/isotp.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/net/can/isotp.c b/net/can/isotp.c
--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -1907,13 +1907,18 @@
 
 	pr_info("can: isotp protocol (max_pdu_size %d)\n", max_pdu_size);
 
+	err = register_netdevice_notifier(&canisotp_notifier);
+	if (err)
+		return err;
+
 	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);
+		unregister_netdevice_notifier(&canisotp_notifier);
+		return err;
+	}
 
-	return err;
+	return 0;
 }
 
 static __exit void isotp_module_exit(void)
-- 
2.25.1

Re: [PATCH net v2] 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:

Pre-existing issues:
- [High] Missing error checks for can_rx_register() in isotp_bind().
--

--- Patch [1]: [PATCH net v2] can: isotp: check register_netdevice_notifier() error in module init ---
commit 059042597b87e317cf8905dce3a8140167d9528a
Author: Minhong He <heminhong@kylinos.cn>

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

This commit registers the netdevice notifier before calling
can_proto_register() and checks its return value. If the protocol
registration fails, it properly unregisters the notifier before
returning the error.

> diff --git a/net/can/isotp.c b/net/can/isotp.c
> index 54becaf6898f1..ae6260e98a7a6 100644
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c

[ ... ]

This is a pre-existing issue, but I noticed a potential problem in
isotp_bind() when reviewing this file:

net/can/isotp.c:isotp_bind() {
	...
	if (isotp_register_rxid(so))
		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
				isotp_rcv, sk, "isotp", sk);

	/* no consecutive frame echo skb in flight */
	so->cfecho = 0;

	/* register for echo skb's */
	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
			isotp_rcv_echo, sk, "isotpe", sk);

	/* switch to new settings */
	so->ifindex = ifindex;
	...
}

[Severity: High]
Does this code silently ignore the return values of can_rx_register()?

If an unprivileged user inside a container calls bind() under memory
pressure, can_rx_register() could fail and return -ENOMEM. Because the
error is not handled, the socket would proceed to be marked as bound.

Later during close, isotp_release() calls can_rx_unregister()
unconditionally. Since the receiver was never successfully registered, this
would trigger the pr_warn("can: receive list entry not found...") inside
af_can.c.

Could a user repeatedly trigger this to generate unbounded kernel log spam,
creating a denial of service vector?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729085656.134523-1-heminhong@kylinos.cn?part=1

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

[Oliver Hartkopp]

On 29.07.26 11:13, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing error checks for can_rx_register() in isotp_bind().
> --
> 
> --- Patch [1]: [PATCH net v2] can: isotp: check register_netdevice_notifier() error in module init ---
> commit 059042597b87e317cf8905dce3a8140167d9528a
> Author: Minhong He <heminhong@kylinos.cn>
> 
> can: isotp: check register_netdevice_notifier() error in module init
> 
> This commit registers the netdevice notifier before calling
> can_proto_register() and checks its return value. If the protocol
> registration fails, it properly unregisters the notifier before
> returning the error.
> 
>> diff --git a/net/can/isotp.c b/net/can/isotp.c
>> index 54becaf6898f1..ae6260e98a7a6 100644
>> --- a/net/can/isotp.c
>> +++ b/net/can/isotp.c
> 
> [ ... ]
> 
> This is a pre-existing issue, but I noticed a potential problem in
> isotp_bind() when reviewing this file:
> 
> net/can/isotp.c:isotp_bind() {
> 	...
> 	if (isotp_register_rxid(so))
> 		can_rx_register(net, dev, rx_id, SINGLE_MASK(rx_id),
> 				isotp_rcv, sk, "isotp", sk);
> 
> 	/* no consecutive frame echo skb in flight */
> 	so->cfecho = 0;
> 
> 	/* register for echo skb's */
> 	can_rx_register(net, dev, tx_id, SINGLE_MASK(tx_id),
> 			isotp_rcv_echo, sk, "isotpe", sk);
> 
> 	/* switch to new settings */
> 	so->ifindex = ifindex;
> 	...
> }
> 
> [Severity: High]
> Does this code silently ignore the return values of can_rx_register()?
> 
> If an unprivileged user inside a container calls bind() under memory
> pressure, can_rx_register() could fail and return -ENOMEM. Because the
> error is not handled, the socket would proceed to be marked as bound.
> 
> Later during close, isotp_release() calls can_rx_unregister()
> unconditionally. Since the receiver was never successfully registered, this
> would trigger the pr_warn("can: receive list entry not found...") inside
> af_can.c.
> 
> Could a user repeatedly trigger this to generate unbounded kernel log spam,
> creating a denial of service vector?
> 

Ok, so no crashes, no memory leaks just spamming the log in the case of 
ENOMEM as the device existence and the fact that it is a CAN device has 
been checked some lines before?

I think when we have a real ENOMEM problem the log output will be our 
smallest problem.

But properly returning -ENOMEM in this case is a good improvement for a 
later patch. I will prepare one when the CAN XL support for ISO-TP patch 
set is discussed. Nothing urgent for now.

Thanks for the review,
Oliver