[PATCH net] can: bcm: add missing synchronize_rcu() in bcm_delete_rx_op()

[Nguyen Le Thanh Tung]

bcm_delete_rx_op(), reachable from an unprivileged user via an RX_DELETE
bcm_msg_head, unregisters the CAN rx handler and then immediately calls
bcm_remove_op(), which cancels op->timer and frees the op via call_rcu():

	can_rx_unregister(..., bcm_rx_handler, op);
	list_del_rcu(&op->list);
	bcm_remove_op(op);		/* hrtimer_cancel() + call_rcu() */

bcm_rx_handler() runs in an RCU read-side critical section and re-arms
op->timer on frame reception via bcm_rx_starttimer(). A handler that is
already in flight on another CPU when RX_DELETE is processed can be past
its RX_NO_AUTOTIMER check -- a plain, unordered u32 store set a few lines
earlier in bcm_delete_rx_op() -- and re-arm the timer after bcm_remove_op()
has cancelled it. The op is then freed after the grace period with its
hrtimer still enqueued, so the timer fires on freed memory in the timer
softirq: an out-of-bounds access that can crash the machine or disclose
adjacent kernel memory. AF_CAN BCM sockets are reachable in a user +
network namespace (e.g. over a vcan interface), so this is triggerable
by an unprivileged local user.

commit d5f9023fa61e ("can: bcm: delay release of struct bcm_op after
synchronize_rcu()") fixed exactly this race for the socket teardown path
in bcm_release(), by draining in-flight handlers with synchronize_rcu()
between can_rx_unregister() and bcm_remove_op(), but the RX_DELETE path in
bcm_delete_rx_op() was left unchanged and still has the race.

Add the same synchronize_rcu() to bcm_delete_rx_op() so that no
bcm_rx_handler() can be running -- and thus re-arm the timer -- once
bcm_remove_op() cancels it and frees the op. bcm_delete_rx_op() is only
called from bcm_sendmsg() under lock_sock() in process context, so
sleeping in synchronize_rcu() is safe here, exactly as it already is in
bcm_release().

Fixes: d5f9023fa61e ("can: bcm: delay release of struct bcm_op after synchronize_rcu()")
Signed-off-by: Nguyen Le Thanh Tung <nguyenlethanhtung555@xxxxxxxxx>
---
 net/can/bcm.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/can/bcm.c b/net/can/bcm.c
index 75653584f..25f868530 100644
--- a/net/can/bcm.c
+++ b/net/can/bcm.c
@@ -822,6 +822,14 @@ static int bcm_delete_rx_op(struct list_head *ops, struct bcm_msg_head *mh,
 						  bcm_rx_handler, op);
 
 			list_del_rcu(&op->list);
+
+			/* wait for any in-flight bcm_rx_handler() to finish so a
+			 * concurrent frame reception cannot re-arm op->timer after
+			 * bcm_remove_op() has cancelled it and the op is freed
+			 * (same drain as the bcm_release() teardown path).
+			 */
+			synchronize_rcu();
+
 			bcm_remove_op(op);
 			return 1; /* done */
 		}
-- 
2.34.1