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

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

[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.

Signed-off-by: Lee Jones <lee@kernel.org>
---
 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;
-- 
2.54.0.rc1.555.g9c883467ad-goog

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@kernel.org>

Many thanks for the investigation and the fix!

Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

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;

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

On 22.04.2026 14:55:50, Oliver Hartkopp wrote:
>
>
> 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@kernel.org>
>
> Many thanks for the investigation and the fix!
>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>

Can we add a Fixes: tag?

regards,
Marc

-- 
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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Oliver Hartkopp]

On 24.04.26 21:08, Marc Kleine-Budde wrote:
> On 22.04.2026 14:55:50, Oliver Hartkopp wrote:
>>
>>
>> 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@kernel.org>
>>
>> Many thanks for the investigation and the fix!
>>
>> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> Can we add a Fixes: tag?

Yes, we should.

Thanks!

Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

Best regards,
Oliver

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

[Lee Jones]

On Sat, 25 Apr 2026, Oliver Hartkopp wrote:

> 
> 
> On 24.04.26 21:08, Marc Kleine-Budde wrote:
> > On 22.04.2026 14:55:50, Oliver Hartkopp wrote:
> > > 
> > > 
> > > 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@kernel.org>
> > > 
> > > Many thanks for the investigation and the fix!
> > > 
> > > Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > 
> > 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?

> Yes, we should.
> 
> Thanks!
> 
> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>

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

-- 
Lee Jones

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

[Oliver Hartkopp]

On 27.04.26 14:40, Lee Jones wrote:
> On Sat, 25 Apr 2026, Oliver Hartkopp wrote:
> 
>>
>>
>> On 24.04.26 21:08, Marc Kleine-Budde wrote:
>>> On 22.04.2026 14:55:50, Oliver Hartkopp wrote:
>>>>
>>>>
>>>> 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@kernel.org>
>>>>
>>>> Many thanks for the investigation and the fix!
>>>>
>>>> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>
>>> 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?
> 
>> Yes, we should.
>>
>> Thanks!
>>
>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> 
> 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

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

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 919 bytes --]

On 27.04.2026 19:15:29, Oliver Hartkopp wrote:
> > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> >
> > 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

> 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.

> 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

-- 
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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Oliver Hartkopp]

On 27.04.26 19:41, Marc Kleine-Budde wrote:
> On 27.04.2026 19:15:29, Oliver Hartkopp wrote:
>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>
>>> 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
> 
>> 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.

>> 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

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

[Lee Jones]

On Mon, 27 Apr 2026, Marc Kleine-Budde wrote:

> On 27.04.2026 19:15:29, Oliver Hartkopp wrote:
> > > > Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
> > >
> > > 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
> 
> > 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.

-- 
Lee Jones

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

[Oliver Hartkopp]

On 28.04.26 10:57, Lee Jones wrote:
> On Mon, 27 Apr 2026, Marc Kleine-Budde wrote:
> 
>> On 27.04.2026 19:15:29, Oliver Hartkopp wrote:
>>>>> Signed-off-by: Oliver Hartkopp <socketcan@hartkopp.net>
>>>>
>>>> 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
>>
>>> 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.
> 

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