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

On 29.07.26 11:13, [email protected] 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 <[email protected]>
> 
> 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