[PATCH net] net/x25: fix use-after-free of the socket by its timers

[PATCH net] net/x25: fix use-after-free of the socket by its timers

[Baul Lee]

x25_start_heartbeat() and the x25->timer helpers arm their timers with a
bare mod_timer() and take no reference on the socket, while
x25_stop_heartbeat() and x25_stop_timer() cancel them with the
non-synchronising timer_delete().  A pending timer therefore keeps
nothing alive, and a cancel does not wait for a callback that is already
running on another CPU.

x25_heartbeat_expiry() rearms itself unconditionally, so an expiry that
races teardown reinstalls sk->sk_timer after __x25_destroy_socket() has
already passed its cancel point.  The following __sock_put() frees the
struct x25_sock while the timer is still queued, and the next expiry
dereferences freed memory.  KASAN reports a slab-use-after-free read in
x25_heartbeat_expiry() below call_timer_fn(), on a kmalloc-2k object
freed by close() on another task.

Switching the cancels to timer_delete_sync() is not the fix: both are
reachable from inside the very timer they would then wait on.
x25_heartbeat_expiry() reaches x25_stop_heartbeat() through
x25_destroy_socket_from_timer() -> __x25_destroy_socket(), and
x25_timer_expiry() reaches x25_stop_timer() through
x25_do_timer_expiry() -> x25_disconnect().  Either would deadlock the
softirq against itself.

Give every armed timer a reference on the socket instead, with
sk_reset_timer() and sk_stop_timer(), and drop that reference in each
expiry handler, which owns the reference of the firing it services.  A
pending or running timer then keeps the socket alive by itself, so the
existing non-synchronising cancels are safe and no path waits on itself.

The heartbeat must also stop rearming once teardown is done with the
socket, or it would keep it alive forever.  __x25_destroy_socket()
unlinks the socket with x25_remove_socket(), which uses
sk_del_node_init(), so sk_hashed() is a reliable marker: rearm only
while the socket is still linked.  A heartbeat that wins the race and
rearms just before the unlink finds the socket unlinked one period later
and lets go, so the socket is released within one heartbeat interval
rather than leaked.

__x25_destroy_socket() also reuses sk->sk_timer as the deferred destroy
timer through a raw add_timer(); arm it with sk_reset_timer() as well
and drop the reference in x25_destroy_timer(), so that path follows the
same rule.

Verified on v7.2-rc4 arm64 with KASAN under QEMU, both kernels built
from the same config and driven by the same connect/close reproducer for
45s.  With the heartbeat period shortened so the microsecond race recurs,
the unpatched kernel panics in __run_timers() on LIST_POISON2; the
patched kernel completes the run at the same race duty with no splat, no
refcount warning, and no sockets left in /proc/net/x25.

Discovered by XBOW, triaged by Baul Lee <baul.lee@xbow.com>

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Federico Kirschbaum <federico.kirschbaum@xbow.com>
Reported-by: Baul Lee <baul.lee@xbow.com>
Cc: stable@vger.kernel.org
Signed-off-by: Baul Lee <baul.lee@xbow.com>
---
 net/x25/af_x25.c    |  4 ++--
 net/x25/x25_timer.c | 25 ++++++++++++++++---------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/net/x25/af_x25.c b/net/x25/af_x25.c
index c31d2af5dd22..5f2fee4da853 100644
--- a/net/x25/af_x25.c
+++ b/net/x25/af_x25.c
@@ -363,6 +363,7 @@ static void x25_destroy_timer(struct timer_list *t)
 	struct sock *sk = timer_container_of(sk, t, sk_timer);
 
 	x25_destroy_socket_from_timer(sk);
+	sock_put(sk);
 }
 
 /*
@@ -398,9 +399,8 @@ static void __x25_destroy_socket(struct sock *sk)
 
 	if (sk_has_allocations(sk)) {
 		/* Defer: outstanding buffers */
-		sk->sk_timer.expires  = jiffies + 10 * HZ;
 		sk->sk_timer.function = x25_destroy_timer;
-		add_timer(&sk->sk_timer);
+		sk_reset_timer(sk, &sk->sk_timer, jiffies + 10 * HZ);
 	} else {
 		/* drop last reference so sock_put will free */
 		__sock_put(sk);
diff --git a/net/x25/x25_timer.c b/net/x25/x25_timer.c
index 2ec63a1f4c6d..7896cd43f1cc 100644
--- a/net/x25/x25_timer.c
+++ b/net/x25/x25_timer.c
@@ -36,45 +36,45 @@ void x25_init_timers(struct sock *sk)
 
 void x25_start_heartbeat(struct sock *sk)
 {
-	mod_timer(&sk->sk_timer, jiffies + 5 * HZ);
+	sk_reset_timer(sk, &sk->sk_timer, jiffies + 5 * HZ);
 }
 
 void x25_stop_heartbeat(struct sock *sk)
 {
-	timer_delete(&sk->sk_timer);
+	sk_stop_timer(sk, &sk->sk_timer);
 }
 
 void x25_start_t2timer(struct sock *sk)
 {
 	struct x25_sock *x25 = x25_sk(sk);
 
-	mod_timer(&x25->timer, jiffies + x25->t2);
+	sk_reset_timer(sk, &x25->timer, jiffies + x25->t2);
 }
 
 void x25_start_t21timer(struct sock *sk)
 {
 	struct x25_sock *x25 = x25_sk(sk);
 
-	mod_timer(&x25->timer, jiffies + x25->t21);
+	sk_reset_timer(sk, &x25->timer, jiffies + x25->t21);
 }
 
 void x25_start_t22timer(struct sock *sk)
 {
 	struct x25_sock *x25 = x25_sk(sk);
 
-	mod_timer(&x25->timer, jiffies + x25->t22);
+	sk_reset_timer(sk, &x25->timer, jiffies + x25->t22);
 }
 
 void x25_start_t23timer(struct sock *sk)
 {
 	struct x25_sock *x25 = x25_sk(sk);
 
-	mod_timer(&x25->timer, jiffies + x25->t23);
+	sk_reset_timer(sk, &x25->timer, jiffies + x25->t23);
 }
 
 void x25_stop_timer(struct sock *sk)
 {
-	timer_delete(&x25_sk(sk)->timer);
+	sk_stop_timer(sk, &x25_sk(sk)->timer);
 }
 
 unsigned long x25_display_timer(struct sock *sk)
@@ -108,7 +108,7 @@ static void x25_heartbeat_expiry(struct timer_list *t)
 			     sock_flag(sk, SOCK_DEAD))) {
 				bh_unlock_sock(sk);
 				x25_destroy_socket_from_timer(sk);
-				return;
+				goto out;
 			}
 			break;
 
@@ -120,8 +120,14 @@ static void x25_heartbeat_expiry(struct timer_list *t)
 			break;
 	}
 restart_heartbeat:
-	x25_start_heartbeat(sk);
+	/* Do not rearm once __x25_destroy_socket() has unlinked the socket:
+	 * it is past its cancel point and owns the teardown from there on.
+	 */
+	if (sk_hashed(sk))
+		x25_start_heartbeat(sk);
 	bh_unlock_sock(sk);
+out:
+	sock_put(sk);
 }
 
 /*
@@ -166,4 +172,5 @@ static void x25_timer_expiry(struct timer_list *t)
 	} else
 		x25_do_timer_expiry(sk);
 	bh_unlock_sock(sk);
+	sock_put(sk);
 }
-- 
2.50.1 (Apple Git-155)