Skip to contentSkip to conversation

can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

PATCH Superseded Lee Jones 2 participants 2 messages May 20

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Lee Jones

author
#0 lore raw

PATCH 1/1 can: bcm: defer rx_op deallocation to workqueue to fix thrtimer UAF

Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()") replaced synchronize_rcu() in bcm_delete_rx_op() with call_rcu() and introduced the RX_NO_AUTOTIMER flag.

However, this flag check was omitted for thrtimer in the packet rx fast-path. During BCM RX operation teardown, a concurrent RCU reader (bcm_rx_handler) can race and re-arm thrtimer via bcm_rx_update_and_send() after call_rcu() has been scheduled. Once the RCU grace period elapses, bcm_op is freed. The subsequently firing thrtimer then dereferences the deallocated op, causing a UAF.

Adding flag checks to the rx fast-path (bcm_rx_update_and_send) does not fully close the TOCTOU race and introduces latency for every CAN frame. Conversely, calling hrtimer_cancel() directly inside the RCU callback (softirq context) is fatal as hrtimer_cancel() can sleep, triggering a "scheduling while atomic" panic.

Resolve this by deferring the timer cancellation and memory free to a dedicated unbound workqueue (bcm_wq). The RCU callback now queues a work item to bcm_wq, which safely cancels both timers and deallocates memory in sleepable process context. A dedicated workqueue is used to prevent system-wide WQ saturation and is cleanly flushed/destroyed on module unload to avoid rmmod page faults.

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

---
Not v2 per-say - this is replaces the following with a different approach:

can: bcm: prevent thrtimer UAF in rx path by checking RX_NO_AUTOTIMER
- https://lore.kernel.org/r/[email protected]
- https://lore.kernel.org/r/[email protected]

 net/can/bcm.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)
net/can/bcm.c +25 -3
index a4bef2c48a55..fa42c0f89f4e 100644
@@ -58,6 +58,7 @@
 #include <linux/can/skb.h>
 #include <linux/can/bcm.h>
 #include <linux/slab.h>
+#include <linux/workqueue.h>
 #include <linux/spinlock.h>
 #include <net/can.h>
 #include <net/sock.h>
@@ -92,6 +93,8 @@ MODULE_ALIAS("can-proto-2");
 
 #define BCM_MIN_NAMELEN CAN_REQUIRED_SIZE(struct sockaddr_can, can_ifindex)
 
+static struct workqueue_struct *bcm_wq;
+
 /*
  * easy access to the first 64 bit of can(fd)_frame payload. cp->data is
  * 64 bit aligned so the offset has to be multiples of 8 which is ensured
@@ -105,6 +108,7 @@ static inline u64 get_u64(const struct canfd_frame *cp, int offset)
 struct bcm_op {
 	struct list_head list;
 	struct rcu_head rcu;
+	struct work_struct work;
 	int ifindex;
 	canid_t can_id;
 	u32 flags;
@@ -793,9 +797,12 @@ static struct bcm_op *bcm_find_op(struct list_head *ops,
 	return NULL;
 }
 
-static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+static void bcm_free_op_work(struct work_struct *work)
 {
-	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+	struct bcm_op *op = container_of(work, struct bcm_op, work);
+
+	hrtimer_cancel(&op->timer);
+	hrtimer_cancel(&op->thrtimer);
 
 	if ((op->frames) && (op->frames != &op->sframe))
 		kfree(op->frames);
@@ -806,6 +813,14 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
 	kfree(op);
 }
 
+static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+{
+	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+
+	INIT_WORK(&op->work, bcm_free_op_work);
+	queue_work(bcm_wq, &op->work);
+}
+
 static void bcm_remove_op(struct bcm_op *op)
 {
 	hrtimer_cancel(&op->timer);
@@ -1839,11 +1854,15 @@ static int __init bcm_module_init(void)
 {
 	int err;
 
+	bcm_wq = alloc_workqueue("can-bcm-wq", WQ_UNBOUND, 0);
+	if (!bcm_wq)
+		return -ENOMEM;
+
 	pr_info("can: broadcast manager protocol\n");
 
 	err = register_pernet_subsys(&canbcm_pernet_ops);
 	if (err)
-		return err;
+		goto register_pernet_failed;
 
 	err = register_netdevice_notifier(&canbcm_notifier);
 	if (err)
@@ -1861,6 +1880,8 @@ static int __init bcm_module_init(void)
 	unregister_netdevice_notifier(&canbcm_notifier);
 register_notifier_failed:
 	unregister_pernet_subsys(&canbcm_pernet_ops);
+register_pernet_failed:
+	destroy_workqueue(bcm_wq);
 	return err;
 }
 
@@ -1869,6 +1890,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);
 }
 
 module_init(bcm_module_init);
Signature · 1 line
2.54.0.631.ge1b05301d1-goog
maintainer
#1 lore raw

Hello Lee,

did you check my changes to your original patch?

https://lore.kernel.org/linux-can/[email protected]/T/#u

I double checked the code with claude.ai (Sonnet 4.6) and it should fix the issue without a workqueue.

Best regards,
Oliver

Quoted 129 lines · Lee Jones

Commit f1b4e32aca08 ("can: bcm: use call_rcu() instead of costly synchronize_rcu()") replaced synchronize_rcu() in bcm_delete_rx_op() with call_rcu() and introduced the RX_NO_AUTOTIMER flag.

However, this flag check was omitted for thrtimer in the packet rx fast-path. During BCM RX operation teardown, a concurrent RCU reader (bcm_rx_handler) can race and re-arm thrtimer via bcm_rx_update_and_send() after call_rcu() has been scheduled. Once the RCU grace period elapses, bcm_op is freed. The subsequently firing thrtimer then dereferences the deallocated op, causing a UAF.

Adding flag checks to the rx fast-path (bcm_rx_update_and_send) does not fully close the TOCTOU race and introduces latency for every CAN frame. Conversely, calling hrtimer_cancel() directly inside the RCU callback (softirq context) is fatal as hrtimer_cancel() can sleep, triggering a "scheduling while atomic" panic.

Resolve this by deferring the timer cancellation and memory free to a dedicated unbound workqueue (bcm_wq). The RCU callback now queues a work item to bcm_wq, which safely cancels both timers and deallocates memory in sleepable process context. A dedicated workqueue is used to prevent system-wide WQ saturation and is cleanly flushed/destroyed on module unload to avoid rmmod page faults.

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

---
Not v2 per-say - this is replaces the following with a different approach:

can: bcm: prevent thrtimer UAF in rx path by checking RX_NO_AUTOTIMER
- https://lore.kernel.org/r/[email protected]
- https://lore.kernel.org/r/[email protected]

  net/can/bcm.c | 28 +++++++++++++++++++++++++---
  1 file changed, 25 insertions(+), 3 deletions(-)
net/can/bcm.c +25 -3
index a4bef2c48a55..fa42c0f89f4e 100644
@@ -58,6 +58,7 @@
  #include <linux/can/skb.h>
  #include <linux/can/bcm.h>
  #include <linux/slab.h>
+#include <linux/workqueue.h>
  #include <linux/spinlock.h>
  #include <net/can.h>
  #include <net/sock.h>
@@ -92,6 +93,8 @@ MODULE_ALIAS("can-proto-2");
 
  #define BCM_MIN_NAMELEN CAN_REQUIRED_SIZE(struct sockaddr_can, can_ifindex)
 
+static struct workqueue_struct *bcm_wq;
+
  /*
   * easy access to the first 64 bit of can(fd)_frame payload. cp->data is
   * 64 bit aligned so the offset has to be multiples of 8 which is ensured
@@ -105,6 +108,7 @@ static inline u64 get_u64(const struct canfd_frame *cp, int offset)
  struct bcm_op {
  	struct list_head list;
  	struct rcu_head rcu;
+	struct work_struct work;
  	int ifindex;
  	canid_t can_id;
  	u32 flags;
@@ -793,9 +797,12 @@ static struct bcm_op *bcm_find_op(struct list_head *ops,
  	return NULL;
  }
 
-static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+static void bcm_free_op_work(struct work_struct *work)
  {
-	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+	struct bcm_op *op = container_of(work, struct bcm_op, work);
+
+	hrtimer_cancel(&op->timer);
+	hrtimer_cancel(&op->thrtimer);
 
  	if ((op->frames) && (op->frames != &op->sframe))
  		kfree(op->frames);
@@ -806,6 +813,14 @@ static void bcm_free_op_rcu(struct rcu_head *rcu_head)
  	kfree(op);
  }
 
+static void bcm_free_op_rcu(struct rcu_head *rcu_head)
+{
+	struct bcm_op *op = container_of(rcu_head, struct bcm_op, rcu);
+
+	INIT_WORK(&op->work, bcm_free_op_work);
+	queue_work(bcm_wq, &op->work);
+}
+
  static void bcm_remove_op(struct bcm_op *op)
  {
  	hrtimer_cancel(&op->timer);
@@ -1839,11 +1854,15 @@ static int __init bcm_module_init(void)
  {
  	int err;
 
+	bcm_wq = alloc_workqueue("can-bcm-wq", WQ_UNBOUND, 0);
+	if (!bcm_wq)
+		return -ENOMEM;
+
  	pr_info("can: broadcast manager protocol\n");
 
  	err = register_pernet_subsys(&canbcm_pernet_ops);
  	if (err)
-		return err;
+		goto register_pernet_failed;
 
  	err = register_netdevice_notifier(&canbcm_notifier);
  	if (err)
@@ -1861,6 +1880,8 @@ static int __init bcm_module_init(void)
  	unregister_netdevice_notifier(&canbcm_notifier);
  register_notifier_failed:
  	unregister_pernet_subsys(&canbcm_pernet_ops);
+register_pernet_failed:
+	destroy_workqueue(bcm_wq);
  	return err;
  }
 
@@ -1869,6 +1890,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);
  }
 
  module_init(bcm_module_init);