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

[Lee Jones <lee@kernel.org>]

On Thu, 21 May 2026, Oliver Hartkopp wrote:

> 
> 
> On 21.05.26 13:07, Lee Jones wrote:
> > On Wed, 20 May 2026, Oliver Hartkopp wrote:
> > > > > Ok, thanks for the detailed explanation!
> > > > > 
> > > > > As your updated patch fixes the original patch
> > > > > 
> > > > > f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly
> > > > > synchronize_rcu()")
> > > > > 
> > > > > we should also revert this op->flags setting that has been introduced with
> > > > > that patch in addition to the rcu stuff, right?
> > > > > 
> > > > > @@ -755,10 +763,13 @@ static int bcm_delete_rx_op(struct list_head *ops,
> > > > > struct bcm_msg_head *mh,
> > > > > 
> > > > >           list_for_each_entry_safe(op, n, ops, list) {
> > > > >                   if ((op->can_id == mh->can_id) && (op->ifindex == ifindex)
> > > > > &&
> > > > >                       (op->flags & CAN_FD_FRAME) == (mh->flags &
> > > > > CAN_FD_FRAME)) {
> > > > > 
> > > > > +                       /* disable automatic timer on frame reception */
> > > > > +                       op->flags |= RX_NO_AUTOTIMER;
> > > > > +
> > > > 
> > > > You mean from v1?  I thought that was NACKed and not applied?
> > > 
> > > No. These two lines were introduced in the original patch you aim to fix. So
> > > when fixing the original "use call_rcu() instead of costly
> > > synchronize_rcu()" patch that introduced the rcu stuff, this now obsolete
> > > op->flags |= RX_NO_AUTOTIMER should be removed too.
> > 
> > Is that not orthogonal to the fix?
> > 
> > If it is, would you be kind enough to submit a fix (and get your own
> > creds up ;-D).  This is usually something that I'd gleefully pick up,
> > but I'm absolutely inundated with vulnerability reports right now.
> > 
> 
> Yes, of course I can clean this up later. But not for getting extra creds
> :-)
> 
> Btw. have you seen the AI remark on your patch that asks about an additional
> rcu_barrier() in bcm_module_exit() ?
> 
> https://netdev-ai.bots.linux.dev/ai-review.html?id=9e8842e7-59a2-4ec5-9f65-1df7973c6170
> 
> says this:
> 
> > @@ -1870,6 +1891,7 @@ static void __exit bcm_module_exit(void)
> >  	can_proto_unregister(&bcm_can_proto);
> >  	unregister_netdevice_notifier(&canbcm_notifier);
> >  	unregister_pernet_subsys(&canbcm_pernet_ops);
> > +	destroy_workqueue(bcm_wq);
> >  }
> 
> Does bcm_module_exit need an rcu_barrier() call before destroy_workqueue?
> Consider this sequence:
> 
> A socket is being closed while the module is unloading:
> 
>   bcm_release()
>     bcm_remove_op()
>       call_rcu(&op->rcu, bcm_free_op_rcu)  // RCU callback scheduled
> 
>   Module refcount reaches zero, rmmod proceeds
> 
>   bcm_module_exit()
>     can_proto_unregister(&bcm_can_proto)
>     unregister_netdevice_notifier(&canbcm_notifier)
>     unregister_pernet_subsys(&canbcm_pernet_ops)
>     destroy_workqueue(bcm_wq)             // bcm_wq destroyed
> 
>   RCU grace period elapses
> 
>   bcm_free_op_rcu()                       // callback fires
>     INIT_WORK(&op->work, bcm_free_op_work)
>     queue_work(bcm_wq, &op->work)         // UAF: bcm_wq already freed
> 
> The RCU callback queues work to bcm_wq, but if the callback hasn't fired
> yet when bcm_module_exit runs, it will try to queue_work to a destroyed
> workqueue. Before this patch, bcm_free_op_rcu called kfree directly without
> using module-global resources, so this wasn't an issue.
> 
> Should bcm_module_exit call rcu_barrier() before destroy_workqueue to
> ensure all pending RCU callbacks complete first?

I hadn't seen it (do you get notifications?  If so, how do I enable
those?), but it is legitimate.  I shall send out a v2 early next week.

-- 
Lee Jones