can: isotp: use unconditional synchronize_rcu() in isotp_release()
Subsystems (2)
Oliver Hartkopp
maintainerPATCH v2 1/1 can: isotp: use unconditional synchronize_rcu() in isotp_release()
isotp_notify() unregisters the (RCU) CAN filters via can_rx_unregister() and clears so->bound without waiting for a grace period. isotp_release() uses so->bound to decide whether it needs to call synchronize_rcu() before cancelling so->rxtimer, so when NETDEV_UNREGISTER runs first it skips that synchronize_rcu() and can cancel the timer while an in-flight isotp_rcv() is still executing and about to re-arm it via isotp_send_fc(), leading to a use-after-free timer callback on the freed socket.
sakisho-bot remarked a problem with rtnl_lock held in isotp_notify(), therefore make isotp_release() always call synchronize_rcu() before cancelling the timers, regardless of so->bound. This still closes the original race (isotp_notify() clearing so->bound without waiting for in-flight isotp_rcv() callers before isotp_release() cancels the RX timer) without adding any RCU wait to the netdevice notifier path.
--- net/can/isotp.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
net/can/isotp.c +8 -1
@@ -1235,15 +1235,22 @@ static int isotp_release(struct socket *sock)can_rx_unregister (net, dev, so->txid,SINGLE_MASK (so->txid), isotp_rcv_echo, sk);dev_put (dev); -synchronize_rcu (); } } } +/* Always wait for a grace period before touching the timers below. +* A concurrent NETDEV_UNREGISTER may have already unregistered our +* filters and cleared so->bound in isotp_notify() without waiting +* for in-flight isotp_rcv() callers to finish, so this call must not +* be skipped just because so->bound is already 0 here. +*/ +synchronize_rcu (); +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->rxtimer); so->ifindex =0 ;
Signature · 1 line
2.53.0
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Skipped CAN filter unregistration during concurrent device removal leads to Use-After-Free and memory leak. --
--- Patch [1]: [PATCH v2] can: isotp: use unconditional synchronize_rcu() in isotp_release() --- commit 0af34892c79e9f6de424253e9d5e275db6cb6280
Author: Oliver Hartkopp <[email protected]>
can: isotp: use unconditional synchronize_rcu() in isotp_release()
This commit addresses a race condition in the timer callback of isotp sockets. It enforces an unconditional synchronize_rcu() in isotp_release() before cancelling timers to avoid issues during concurrent NETDEV_UNREGISTER notifications.
Quoted 3 lines
net/can/isotp.c +0
-0
[ ... ]
net/can/isotp.c static int isotp_release(struct socket *sock) · line 1237
Quoted 12 lines
net/can/isotp.c +0
-0@@ -1227,14 +1227,20 @@ static int isotp_release(struct socket *sock)/* remove current filters & unregister */ if (so->bound) {if (so->ifindex) {struct net_device *dev; dev =dev_get_by_index (net, so->ifindex);if (dev) {if (isotp_register_rxid (so))can_rx_unregister (net, dev, so->rxid,SINGLE_MASK (so->rxid), isotp_rcv, sk);
[Severity: Critical]
This is a pre-existing issue, but could this dev_get_by_index() call return NULL if a device unregistration is running concurrently?
If unregister_netdevice_many() unlists the device before the NETDEV_UNREGISTER notifier executes, and isotp_release() is called at the same time, it removes the socket from the isotp notifier list.
When it calls dev_get_by_index() here, it will return NULL because the device is unlisted. The if (dev) condition fails and we skip calling can_rx_unregister().
The NETDEV_UNREGISTER notifier will miss this socket because it was already removed from the notifier list.
Since the socket is freed at the end of this function, does this leave an active CAN filter pointing to a freed socket? Any delayed or looped-back CAN frames processed during device teardown could invoke isotp_rcv() on this freed socket, causing a use-after-free and a memory leak.
Quoted 12 lines
net/can/isotp.c +2
-1can_rx_unregister (net, dev, so->txid,SINGLE_MASK (so->txid), isotp_rcv_echo, sk);dev_put (dev); -synchronize_rcu (); } } } +/* Always wait for a grace period before touching the timers below. +* A concurrent NETDEV_UNREGISTER may have already unregistered our
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Oliver Hartkopp
maintainerHi Marc,
do you consider to upstream this patch together with the BCM patch set?
I know sashiko-bot had again some "Pre-existing issue" remarked on this patch. But this simple fix should probably go into upstream, before the next "AI Pre-existing issue"-party starts with isotp.c and sashiko-bot ;-)
Best regards,
Oliver
Quoted 52 lines · Oliver Hartkopp
isotp_notify()unregisters the (RCU) CAN filters viacan_rx_unregister()and clearsso->boundwithout waiting for a grace period.isotp_release()usesso->boundto decide whether it needs to callsynchronize_rcu()before cancellingso->rxtimer, so whenNETDEV_UNREGISTERruns first it skips thatsynchronize_rcu()and can cancel the timer while an in-flightisotp_rcv()is still executing and about to re-arm it viaisotp_send_fc(), leading to a use-after-free timer callback on the freed socket.sakisho-bot remarked a problem with
rtnl_lockheld inisotp_notify(), therefore makeisotp_release()always callsynchronize_rcu()before cancelling the timers, regardless ofso->bound. This still closes the original race (isotp_notify()clearingso->boundwithout waiting for in-flightisotp_rcv()callers beforeisotp_release()cancels the RX timer) without adding any RCU wait to the netdevice notifier path.Fixes 14a4696bc311 ("can: isotp: isotp_release(): omit unintended hrtimer restart on socket release")Reported-by Nico Yip <[email protected]> (ZDI-CAN-31764)Signed-off-by Oliver Hartkopp <[email protected]>--- net/can/isotp.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)net/can/isotp.c +8
-1@@ -1235,15 +1235,22 @@ static int isotp_release(struct socket *sock)can_rx_unregister (net, dev, so->txid,SINGLE_MASK (so->txid), isotp_rcv_echo, sk);dev_put (dev); -synchronize_rcu (); } } } +/* Always wait for a grace period before touching the timers below. +* A concurrent NETDEV_UNREGISTER may have already unregistered our +* filters and cleared so->bound in isotp_notify() without waiting +* for in-flight isotp_rcv() callers to finish, so this call must not +* be skipped just because so->bound is already 0 here. +*/ +synchronize_rcu (); +hrtimer_cancel (&so->txfrtimer);hrtimer_cancel (&so->txtimer);hrtimer_cancel (&so->rxtimer); so->ifindex =0 ;