Re: [PATCH 1/1] can: bcm: prevent thrtimer UAF in rx path by checking RX_NO_AUTOTIMER

[Oliver Hartkopp]

On 22.04.26 12:22, Lee Jones wrote:
> Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
> synchronize_rcu()") removed the synchronize_rcu() call from
> bcm_delete_rx_op() and introduced the RX_NO_AUTOTIMER flag to prevent
> timers from being rearmed during deletion.  However, it only applied
> this check to op->timer via bcm_rx_starttimer().
>
> It missed the fact that op->thrtimer can also be rearmed by an
> in-flight bcm_rx_handler() (which runs as an RCU reader) via
> bcm_rx_update_and_send().  This allows op->thrtimer to be queued after
> bcm_remove_op() has already cancelled it, leading to a use-after-free
> when the timer fires on the deferred-freed struct bcm_op.
>
> Address the omission by checking the RX_NO_AUTOTIMER flag
> in bcm_rx_update_and_send() before starting op->thrtimer, effectively
> preventing it from being rearmed concurrently with teardown.
>
> Signed-off-by: Lee Jones <lee@xxxxxxxxxx>

Many thanks for the investigation and the fix!

Acked-by: Oliver Hartkopp <socketcan@xxxxxxxxxxxx>

Best regards,
Oliver

> ---
>   net/can/bcm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/net/can/bcm.c b/net/can/bcm.c
> index a4bef2c48a55..67e5b3149a8f 100644
> --- a/net/can/bcm.c
> +++ b/net/can/bcm.c
> @@ -539,6 +539,12 @@ static void bcm_rx_update_and_send(struct bcm_op *op,
>   	if (hrtimer_active(&op->thrtimer))
>   		return;
>   
> +	/* bcm_remove_op() may have cancelled thrtimer concurrently with this
> +	 * RCU-protected handler; do not rearm it. Mirrors bcm_rx_starttimer().
> +	 */
> +	if (op->flags & RX_NO_AUTOTIMER)
> +		return;
> +
>   	/* first reception with enabled throttling mode */
>   	if (!op->kt_lastmsg)
>   		goto rx_changed_settime;