can: bcm: prevent thrtimer UAF in rx path by checking RX_NO_AUTOTIMER

PATCH Applied Lee Jones 3 participants 12 messages Apr 22 – May 6

Subsystems (2)

CAN network drivers CAN network layer

lore

Share & tools

lore

Lee Jones

author

2026-04-22 10:22 UTC #0 lore raw

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

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 <[email protected]>

---
 net/can/bcm.c | 6 ++++++
 1 file changed, 6 insertions(+)

net/can/bcm.c +6 -0

index a4bef2c48a55..67e5b3149a8f 100644
@@ -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;

Signature · 1 line

2.54.0.rc1.555.g9c883467ad-goog

Oliver Hartkopp

maintainer Acked-by on v1 1/1

2026-04-22 12:56 UTC #1 lore raw

Quoted 12 lines · Lee Jones

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.

Quoted 5 lines

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 <[email protected]>

Many thanks for the investigation and the fix!

Acked-by Oliver Hartkopp <[email protected]>

Best regards,
Oliver

Quoted 21 lines

---
  net/can/bcm.c | 6 ++++++
  1 file changed, 6 insertions(+)

net/can/bcm.c +6 -0

index a4bef2c48a55..67e5b3149a8f 100644
@@ -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;

Marc Kleine-Budde

maintainer

2026-04-24 19:08 UTC #2 lore raw

Quoted 19 lines · Oliver Hartkopp

Quoted 16 lines · Lee Jones
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.

Quoted 5 lines

Quoted 1 line
Signed-off-by Lee Jones <[email protected]>
Many thanks for the investigation and the fix!
Acked-by Oliver Hartkopp <[email protected]>

Can we add a Fixes: tag?

regards,
Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Oliver Hartkopp

maintainer

2026-04-25 06:49 UTC #3 lore raw

Quoted 20 lines · Marc Kleine-Budde

Quoted 19 lines · Oliver Hartkopp
Quoted 16 lines · Lee Jones
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.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Lee Jones <[email protected]>
Many thanks for the investigation and the fix!
Acked-by Oliver Hartkopp <[email protected]>
Can we add a Fixes: tag?

Yes, we should.

Thanks!

Signed-off-by Oliver Hartkopp <[email protected]>

Best regards,
Oliver

Lee Jones

author

2026-04-27 12:41 UTC #4 lore raw

Quoted 23 lines · Oliver Hartkopp

Quoted 20 lines · Marc Kleine-Budde
Quoted 19 lines · Oliver Hartkopp
Quoted 16 lines · Lee Jones
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.

Quoted 7 lines

Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Lee Jones <[email protected]>
Many thanks for the investigation and the fix!
Acked-by Oliver Hartkopp <[email protected]>
Can we add a Fixes: tag?

Fixes f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")

Do you need me to resubmit or are you okay to apply manually / with b4?

Quoted 5 lines

Yes, we should.
Thanks!
Signed-off-by Oliver Hartkopp <[email protected]>

I'm a little confused by the SoB. Does this mean you've applied it?

Signature · 1 line

Lee Jones

Oliver Hartkopp

maintainer

2026-04-27 17:18 UTC #5 lore raw

Quoted 37 lines · Lee Jones

Quoted 23 lines · Oliver Hartkopp
Quoted 20 lines · Marc Kleine-Budde
Quoted 19 lines · Oliver Hartkopp
Quoted 16 lines · Lee Jones
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.
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
Signed-off-by Lee Jones <[email protected]>
Many thanks for the investigation and the fix!
Acked-by Oliver Hartkopp <[email protected]>
Can we add a Fixes: tag?
Fixes f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()")
Do you need me to resubmit or are you okay to apply manually / with b4?

Quoted 8 lines

Quoted 5 lines
Yes, we should.
Thanks!
Signed-off-by Oliver Hartkopp <[email protected]>
I'm a little confused by the SoB. Does this mean you've applied it?

No. Marc will apply the patch.

My SoB is probably not needed in this context. But Marc was sometimes asking for it when I commented and discussed patches in the past.

I also thought Marc would add the Fixes tag on his own.

Best regards,
Oliver

Marc Kleine-Budde

maintainer

2026-04-27 17:41 UTC #6 lore raw

Quoted 8 lines · Oliver Hartkopp

Quoted 4 lines
Quoted 1 line
Signed-off-by Oliver Hartkopp <[email protected]>
I'm a little confused by the SoB. Does this mean you've applied it?
No. Marc will apply the patch.
My SoB is probably not needed in this context.

ACK

Quoted 2 lines

But Marc was sometimes asking
for it when I commented and discussed patches in the past.

The rule is: if you post a patch, your S-o-b has to be the last S-o-b in the line.

Quoted 1 line

I also thought Marc would add the Fixes tag on his own.

If you add it in the thread, b-4 will pick it up automatically and this less work for me :)

regards,
Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Oliver Hartkopp

maintainer

2026-04-27 18:01 UTC #7 lore raw

Quoted 10 lines · Marc Kleine-Budde

Quoted 8 lines · Oliver Hartkopp
Quoted 4 lines
Quoted 1 line
Signed-off-by Oliver Hartkopp <[email protected]>
I'm a little confused by the SoB. Does this mean you've applied it?
No. Marc will apply the patch.
My SoB is probably not needed in this context.

Quoted 8 lines

ACK
Quoted 2 lines
But Marc was sometimes asking
for it when I commented and discussed patches in the past.
The rule is: if you post a patch, your S-o-b has to be the last S-o-b in the line.

Ok.

Quoted 4 lines

Quoted 1 line
I also thought Marc would add the Fixes tag on his own.
If you add it in the thread, b-4 will pick it up automatically and this less work for me :)

Ah. This time Lee already did the job for b4 ;-)

Best regards,
Oliver

Lee Jones

author

2026-04-28 08:57 UTC #8 lore raw

In reply to Marc Kleine-Budde

Quoted 10 lines · Marc Kleine-Budde

Quoted 8 lines · Oliver Hartkopp
Quoted 4 lines
Quoted 1 line
Signed-off-by Oliver Hartkopp <[email protected]>
I'm a little confused by the SoB. Does this mean you've applied it?
No. Marc will apply the patch.
My SoB is probably not needed in this context.

Quoted 7 lines

ACK
Quoted 2 lines
But Marc was sometimes asking
for it when I commented and discussed patches in the past.
The rule is: if you post a patch, your S-o-b has to be the last S-o-b in the line.

Right. But SoBs are an indicator that a given person was part of the patch flow; author, submitter, sub-maintainer, maintainer, etc.

In this context Oliver's SoB should have been Reviewed-by or Acked-by.

Thanks for confirming that you use b4 - that makes life easier.

Signature · 1 line

Lee Jones

Oliver Hartkopp

maintainer

2026-04-28 09:33 UTC #9 lore raw

Quoted 17 lines · Lee Jones

Quoted 15 lines · Marc Kleine-Budde
Quoted 8 lines · Oliver Hartkopp
Quoted 4 lines
Quoted 1 line
Signed-off-by Oliver Hartkopp <[email protected]>
I'm a little confused by the SoB. Does this mean you've applied it?
No. Marc will apply the patch.
My SoB is probably not needed in this context.
ACK
Quoted 2 lines
But Marc was sometimes asking
for it when I commented and discussed patches in the past.

Quoted 8 lines

Quoted 2 lines
The rule is: if you post a patch, your S-o-b has to be the last S-o-b in the line.
Right. But SoBs are an indicator that a given person was part of the patch flow; author, submitter, sub-maintainer, maintainer, etc.
In this context Oliver's SoB should have been Reviewed-by or Acked-by.

Yes.

I'm author and maintainer of the bcm code - but Marc manages the patch flow for the CAN subsystem. That's why I sent my Acked-by instantly - which was the right reaction. Sorry for the SoB noise.

Many thanks,
OLiver

Lee Jones

author

2026-05-05 14:42 UTC #10 lore raw

In reply to Marc Kleine-Budde

Quoted 15 lines · Marc Kleine-Budde

Quoted 8 lines · Oliver Hartkopp
Quoted 4 lines
Quoted 1 line
Signed-off-by Oliver Hartkopp <[email protected]>
I'm a little confused by the SoB. Does this mean you've applied it?
No. Marc will apply the patch.
My SoB is probably not needed in this context.
ACK
Quoted 2 lines
But Marc was sometimes asking
for it when I commented and discussed patches in the past.

Quoted 7 lines

The rule is: if you post a patch, your S-o-b has to be the last S-o-b in the line.
Quoted 1 line
I also thought Marc would add the Fixes tag on his own.
If you add it in the thread, b-4 will pick it up automatically and this less work for me :)

Any luck with this one Marc.

This should be send to the -rcs ASAP please.

Signature · 1 line

Lee Jones

Marc Kleine-Budde

maintainer Applied

2026-05-06 12:45 UTC #11 lore raw

In reply to Lee Jones

Quoted 12 lines · Lee Jones

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.

Quoted 5 lines

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 <[email protected]>

Applied to linux-can.

Thanks,
Marc

Signature · 4 lines

Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |