[PATCH net] net: rose: stop timers before freeing neighbour in rose_neigh_put

[PATCH net] net: rose: stop timers before freeing neighbour in rose_neigh_put

[Mashiro Chen]

rose_neigh_put() frees the rose_neigh object when the reference count
reaches zero, but does not stop the t0timer and ftimer beforehand.
If a timer has been scheduled and fires after the object is freed,
the callback will access already-freed memory, leading to a
use-after-free.

For example, this can happen when rose_timer_expiry() in ROSE_STATE_2
calls rose_neigh_put() dropping the last reference while the
neighbour's t0timer is still pending:

  rose_timer_expiry() (ROSE_STATE_2)
    rose_neigh_put(rose->neighbour)   --> refcount drops to 0, kfree
  ...
  rose_t0timer_expiry()               --> UAF: accesses freed neigh

Fix this by calling timer_delete_sync() for both t0timer and ftimer
in rose_neigh_put() before freeing the object. This ensures any
pending or running timer callback completes before the memory is
released.

This is safe because rose_neigh_put() is never called from the
neigh's own timer callbacks (t0timer_expiry / ftimer_expiry), so
timer_delete_sync() cannot deadlock.

Reported-by: syzbot+abd2b69348e2d9b107a1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=abd2b69348e2d9b107a1
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 include/net/rose.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/rose.h b/include/net/rose.h
index 2b5491bbf39ab..c69024e865456 100644
--- a/include/net/rose.h
+++ b/include/net/rose.h
@@ -160,6 +160,8 @@ static inline void rose_neigh_hold(struct rose_neigh *rose_neigh)
 static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
 {
 	if (refcount_dec_and_test(&rose_neigh->use)) {
+		timer_delete_sync(&rose_neigh->t0timer);
+		timer_delete_sync(&rose_neigh->ftimer);
 		if (rose_neigh->ax25)
 			ax25_cb_put(rose_neigh->ax25);
 		kfree(rose_neigh->digipeat);
-- 
2.53.0

[PATCH net v2] net: rose: stop timers before freeing neighbour in rose_neigh_put

[Mashiro Chen]

rose_neigh_put() frees the rose_neigh object when the reference count
reaches zero, but does not stop the t0timer and ftimer beforehand.
If a timer has been scheduled and fires after the object is freed,
the callback will access already-freed memory, leading to a
use-after-free.

For example, this can happen when rose_timer_expiry() in ROSE_STATE_2
calls rose_neigh_put() dropping the last reference while the
neighbour's t0timer is still pending:

  rose_timer_expiry() (ROSE_STATE_2)
    rose_neigh_put(rose->neighbour)   --> refcount drops to 0, kfree
  ...
  rose_t0timer_expiry()               --> UAF: accesses freed neigh

Fix this by calling timer_delete_sync() for both t0timer and ftimer
in rose_neigh_put() before freeing the object. This ensures any
pending or running timer callback completes before the memory is
released.

This is safe because rose_neigh_put() is never called from the
neigh's own timer callbacks (t0timer_expiry / ftimer_expiry), so
timer_delete_sync() cannot deadlock.

The UAF can be triggered through the following sequence:

1. rose_add_node() creates a rose_neigh with refcount=2
   (initial neigh_list reference plus one held by the new node entry)
2. A socket connects via rose_get_neigh(), rose_neigh_hold() raises
   refcount to 3; rose_transmit_link() arms t0timer on the neigh
3. The routing entry is deleted: rose_del_node() drops the node
   reference (3->2), calls rose_remove_neigh() which stops both
   timers, then drops the neigh_list reference (2->1); the socket
   now holds the only reference with t0timer stopped
4. T1 expires in ROSE_STATE_1: rose_timer_expiry() sends
   CLEAR_REQUEST via rose_transmit_link(), which sees
   neigh->restarted=0 and !rose_t0timer_running, and re-arms
   t0timer on the neigh (refcount still 1)
5. T3 expires in ROSE_STATE_2: rose_timer_expiry() calls
   rose_neigh_put(), refcount drops to 0, kfree(neigh)
6. t0timer fires, rose_t0timer_expiry() accesses freed neigh -> UAF

Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Reported-by: syzbot+abd2b69348e2d9b107a1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=abd2b69348e2d9b107a1
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
 include/net/rose.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/net/rose.h b/include/net/rose.h
index 2b5491bbf39ab..c69024e865456 100644
--- a/include/net/rose.h
+++ b/include/net/rose.h
@@ -160,6 +160,8 @@ static inline void rose_neigh_hold(struct rose_neigh *rose_neigh)
 static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
 {
 	if (refcount_dec_and_test(&rose_neigh->use)) {
+		timer_delete_sync(&rose_neigh->t0timer);
+		timer_delete_sync(&rose_neigh->ftimer);
 		if (rose_neigh->ax25)
 			ax25_cb_put(rose_neigh->ax25);
 		kfree(rose_neigh->digipeat);
-- 
2.53.0

[PATCH net v3] net: rose: defer rose_neigh cleanup to workqueue to fix UAF

[Mashiro Chen]

rose_neigh_put() frees the rose_neigh object when the reference count
reaches zero, but does not stop the t0timer and ftimer beforehand.
If a timer has been scheduled and fires after the object is freed,
the callback will access already-freed memory, leading to a
use-after-free.

The UAF can be triggered through the following sequence:

1. rose_add_node() creates a rose_neigh with refcount=2
   (initial neigh_list reference plus one held by the new node entry)
2. A socket connects via rose_get_neigh(), rose_neigh_hold() raises
   refcount to 3; rose_transmit_link() arms t0timer on the neigh
3. The routing entry is deleted: rose_del_node() drops the node
   reference (3->2), calls rose_remove_neigh() which stops both
   timers, then drops the neigh_list reference (2->1); the socket
   now holds the only reference with t0timer stopped
4. T1 expires in ROSE_STATE_1: rose_timer_expiry() sends
   CLEAR_REQUEST via rose_transmit_link(), which sees
   neigh->restarted=0 and !rose_t0timer_running, and re-arms
   t0timer on the neigh (refcount still 1)
5. T3 expires in ROSE_STATE_2: rose_timer_expiry() calls
   rose_neigh_put(), refcount drops to 0, kfree(neigh)
6. t0timer fires, rose_t0timer_expiry() accesses freed neigh -> UAF

Calling timer_delete_sync() directly in rose_neigh_put() would fix
the UAF but is unsafe: rose_neigh_put() can be called while holding
spinlocks (e.g. rose_route_frame() holds rose_neigh_list_lock and
rose_route_list_lock), and timer_delete_sync() may block waiting for
a running callback, which is illegal in atomic context.

Fix this by deferring the final cleanup to a workqueue. When the
refcount drops to zero, schedule rose_neigh_free_work() instead of
freeing inline. The work function runs in process context where it
is safe to call timer_shutdown_sync() before freeing the object.
timer_shutdown_sync() is used instead of timer_delete_sync() to
permanently prevent the self-rearming t0timer from firing again
after the work function starts.

Fixes: d860d1faa6b2 ("net: rose: convert 'use' field to refcount_t")
Reported-by: syzbot+abd2b69348e2d9b107a1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=abd2b69348e2d9b107a1
Signed-off-by: Mashiro Chen <mashiro.chen@mailbox.org>
---
Changes in v3:
  - Replaced timer_delete_sync() in rose_neigh_put() with a workqueue
    to avoid calling a blocking function in atomic context
  - Added rose_neigh_free_work() to handle cleanup in process context
  - Added struct work_struct free_work to struct rose_neigh
  - Used timer_shutdown_sync() to prevent the self-rearming t0timer
    from firing again after cleanup begins

Changes in v2:
  - Stop t0timer and ftimer before freeing the object

 include/net/rose.h    | 10 ++++------
 net/rose/rose_route.c | 16 ++++++++++++++++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/net/rose.h b/include/net/rose.h
index 2b5491bbf39ab..704214e38ef1d 100644
--- a/include/net/rose.h
+++ b/include/net/rose.h
@@ -9,6 +9,7 @@
 #define _ROSE_H 
 
 #include <linux/refcount.h>
+#include <linux/workqueue.h>
 #include <linux/rose.h>
 #include <net/ax25.h>
 #include <net/sock.h>
@@ -105,6 +106,7 @@ struct rose_neigh {
 	struct sk_buff_head	queue;
 	struct timer_list	t0timer;
 	struct timer_list	ftimer;
+	struct work_struct	free_work;
 };
 
 struct rose_node {
@@ -159,12 +161,8 @@ static inline void rose_neigh_hold(struct rose_neigh *rose_neigh)
 
 static inline void rose_neigh_put(struct rose_neigh *rose_neigh)
 {
-	if (refcount_dec_and_test(&rose_neigh->use)) {
-		if (rose_neigh->ax25)
-			ax25_cb_put(rose_neigh->ax25);
-		kfree(rose_neigh->digipeat);
-		kfree(rose_neigh);
-	}
+	if (refcount_dec_and_test(&rose_neigh->use))
+		schedule_work(&rose_neigh->free_work);
 }
 
 /* af_rose.c */
diff --git a/net/rose/rose_route.c b/net/rose/rose_route.c
index e31842e6b3c8b..8c8e09793d442 100644
--- a/net/rose/rose_route.c
+++ b/net/rose/rose_route.c
@@ -28,6 +28,7 @@
 #include <linux/mm.h>
 #include <linux/interrupt.h>
 #include <linux/notifier.h>
+#include <linux/workqueue.h>
 #include <linux/init.h>
 #include <net/rose.h>
 #include <linux/seq_file.h>
@@ -44,6 +45,19 @@ static DEFINE_SPINLOCK(rose_route_list_lock);
 
 struct rose_neigh *rose_loopback_neigh;
 
+static void rose_neigh_free_work(struct work_struct *work)
+{
+	struct rose_neigh *rose_neigh =
+		container_of(work, struct rose_neigh, free_work);
+
+	timer_shutdown_sync(&rose_neigh->t0timer);
+	timer_shutdown_sync(&rose_neigh->ftimer);
+	if (rose_neigh->ax25)
+		ax25_cb_put(rose_neigh->ax25);
+	kfree(rose_neigh->digipeat);
+	kfree(rose_neigh);
+}
+
 /*
  *	Add a new route to a node, and in the process add the node and the
  *	neighbour if it is new.
@@ -103,6 +117,7 @@ static int __must_check rose_add_node(struct rose_route_struct *rose_route,
 
 		timer_setup(&rose_neigh->ftimer, NULL, 0);
 		timer_setup(&rose_neigh->t0timer, NULL, 0);
+		INIT_WORK(&rose_neigh->free_work, rose_neigh_free_work);
 
 		if (rose_route->ndigis != 0) {
 			rose_neigh->digipeat =
@@ -388,6 +403,7 @@ void rose_add_loopback_neigh(void)
 
 	timer_setup(&sn->ftimer, NULL, 0);
 	timer_setup(&sn->t0timer, NULL, 0);
+	INIT_WORK(&sn->free_work, rose_neigh_free_work);
 
 	spin_lock_bh(&rose_neigh_list_lock);
 	sn->next = rose_neigh_list;
-- 
2.53.0