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

On 27.07.26 10:37, Nguyen Le Thanh Tung wrote:
> 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 <[email protected]>

NAK

This is already fixed with

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/net/can/bcm.c?id=68973f9db76144825e4f35dfdc80fb8279eb2d57
("can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF")

without re-introducing the costly synchronize_rcu().

There are a bunch of bcm fixes that have been applied to 7.2-rc3 and are 
queued for several stable trees. Please re-check the current bcm.c 
developments and fixes.

Best regards,
Oliver

> ---
>   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 */
>   		}