[PATCH net] net: x25: shut down T20 timer before freeing neighbour

[PATCH net] net: x25: shut down T20 timer before freeing neighbour

[Felix Hoffmann]

x25_link_terminated() cancels the T20 restart timer with
timer_delete(). If the callback is already running on another CPU, the
cancel does not wait. The callback can then rearm the timer before
x25_link_device_down() removes the neighbour and drops its last
reference. This frees the neighbour while its embedded timer is active,
leading to a use-after-free on expiry.

Shut the timer down synchronously in __x25_remove_neigh() before
dropping the list-owned reference. timer_shutdown_sync() also prevents
the callback's unconditional rearm. The callback does not acquire
x25_neigh_list_lock, so it is safe to wait while neighbour removal keeps
the list protected.

Also purge queued frames before releasing the neighbour, restoring the
cleanup removed by commit 7eed751b3b2a ("net/x25: handle additional
netdev events").

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Felix Hoffmann <f3lix.dev@gmx.de>
---
Tested with a two-vCPU KASAN, timer debugobjects and lockdep kernel using
a deterministic callback/teardown race. All 10 timer callbacks completed
without a KASAN, debugobjects, lockdep, warning, oops, or panic report.

Build-tested with W=1 under allmodconfig and allyesconfig.

The reproducer is available privately on request.

 net/x25/x25_link.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
index d1e8cd81c214..876333cb16ae 100644
--- a/net/x25/x25_link.c
+++ b/net/x25/x25_link.c
@@ -298,6 +298,9 @@ void x25_link_device_up(struct net_device *dev)
  */
 static void __x25_remove_neigh(struct x25_neigh *nb)
 {
+	timer_shutdown_sync(&nb->t20timer);
+	skb_queue_purge(&nb->queue);
+
 	if (nb->node.next) {
 		list_del(&nb->node);
 		x25_neigh_put(nb);
-- 
2.43.0

Re: [PATCH net] net: x25: shut down T20 timer before freeing neighbour

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260731173955.1601698-1-f3lix.dev@gmx.de?part=1
---
--- Patch [1]: [PATCH net] net: x25: shut down T20 timer before freeing neighbour ---
commit 918c9c3f4476f4a299dc5fd66bd610293c1355e6
Author: Felix Hoffmann <f3lix.dev@gmx.de>

net: x25: shut down T20 timer before freeing neighbour

This adds timer_shutdown_sync() on nb->t20timer and skb_queue_purge() on
nb->queue in __x25_remove_neigh(), before the list-owned reference is
dropped, to close a use-after-free where the T20 callback rearms the timer
after x25_link_terminated() cancelled it with timer_delete(). It also
restores the queue purge that commit 7eed751b3b2a removed.

A few questions about the placement of the new cleanup.

> diff --git a/net/x25/x25_link.c b/net/x25/x25_link.c
> index d1e8cd81c2143..876333cb16aee 100644
> --- a/net/x25/x25_link.c
> +++ b/net/x25/x25_link.c
> @@ -298,6 +298,9 @@ void x25_link_device_up(struct net_device *dev)
>   */
>  static void __x25_remove_neigh(struct x25_neigh *nb)
>  {
> +	timer_shutdown_sync(&nb->t20timer);
> +	skb_queue_purge(&nb->queue);
> +

[Severity: High]
[Finding: main-4-5]
[Sources: opus-5]
This isn't a bug introduced by this patch, but with the teardown now
looking complete in __x25_remove_neigh(), is nb->dev still left dangling
for neighbours that outlive their list membership?

x25_link_device_down() drops the device reference right after the removal
helper returns, while __x25_remove_neigh() only releases the list-owned
neighbour reference:

net/x25/x25_link.c:x25_link_device_down() {
	if (nb->dev == dev) {
		__x25_remove_neigh(nb);
		dev_put(dev);
	}
}

x25_link_device_down() does not call x25_kill_by_neigh(), so a socket that
already holds its own reference keeps nb alive:

net/x25/af_x25.c:x25_connect() {
	x25->neighbour = x25_get_neigh(rt->dev);
}

and the same in x25_rx_call_request() via x25_neigh_hold(nb). The
x25_kill_by_neigh() scan that runs on NETDEV_DOWN reads x25_sk(s)->neighbour
under x25_list_lock only, so a socket that assigns x25->neighbour after the
scan passed it is not evicted. NETDEV_UNREGISTER then removes the neighbour
and drops the device reference, leaving nb->dev unheld.

Once the netdev is freed, does the next transmit on that socket read freed
memory here?

net/x25/x25_dev.c:x25_establish_link() {
	switch (nb->dev->type) {
	...
	skb->dev      = nb->dev;

	dev_queue_xmit(skb);
}

x25_send_frame() dereferences nb->dev the same way, reached from
x25_transmit_link() for X25_LINK_STATE_3. Would it make more sense to
invalidate nb->dev (or evict socket references with x25_kill_by_neigh())
at removal time rather than only dropping the device reference?

[Severity: Medium]
[Finding: main-1-0]
[Sources: opus-5]
This is a pre-existing issue, but does the purge belong at list removal
rather than at the neighbour's release point?

x25_neigh_put() frees the object with a bare kfree() and never touches
nb->queue:

include/net/x25.h:x25_neigh_put() {
	if (refcount_dec_and_test(&nb->refcnt))
		kfree(nb);
}

A socket holding its own reference (x25->neighbour) can still reach
x25_transmit_link() through x25_kick()/x25_send_iframe()/x25_write_internal()
after the neighbour has been unlinked and purged:

net/x25/x25_link.c:x25_transmit_link() {
	case X25_LINK_STATE_0:
		skb_queue_tail(&nb->queue, skb);
		nb->state = X25_LINK_STATE_1;
		x25_establish_link(nb);
	case X25_LINK_STATE_1:
	case X25_LINK_STATE_2:
		skb_queue_tail(&nb->queue, skb);
}

No lock is held there, and the remaining drain/purge sites
(x25_link_control(), x25_link_established(), x25_link_terminated()) are only
reachable with a neighbour obtained from x25_get_neigh(), i.e. one that is
still on x25_neigh_list. With the T20 timer now permanently shut down, can
those frames ever be drained again, or do they leak together with the socket
references they carry from sock_alloc_send_skb() in x25_output() when
kfree(nb) runs?

Would moving the purge and the timer shutdown into a release helper called
when the refcount hits zero avoid this, and also avoid leaving a
backporter with the impression that nb->queue is empty at free time?

[Severity: Low]
[Finding: main-2-1]
[Sources: opus-5, opus-4-7]
This isn't a bug, but the kernel-doc immediately above the function now
describes less than the function does:

net/x25/x25_link.c
/**
 *	__x25_remove_neigh - remove neighbour from x25_neigh_list
 *	@nb: - neigh to remove
 *
 *	Remove neighbour from x25_neigh_list. If it was there.
 *	Caller must hold x25_neigh_list_lock.
 */

The function is no longer only a list operation. timer_shutdown_sync() is
irreversible per its own documentation in kernel/time/timer.c ("@timer
cannot be enqueued again. Any attempt to rearm @timer is silently
ignored"), which __mod_timer() implements with:

kernel/time/timer.c:__mod_timer() {
	if (!timer->function)
		goto out_unlock;
}

and skb_queue_purge() destroys frames that x25_transmit_link() queued for
later transmission. The helper also now spin-waits for a callback that may
be running on another CPU, while both callers hold the lock with BH
disabled:

net/x25/x25_link.c:x25_link_device_down() {
	write_lock_bh(&x25_neigh_list_lock);

	list_for_each_safe(entry, tmp, &x25_neigh_list) {
}

x25_link_free() does the same. Could the comment be updated to describe the
synchronous, irreversible teardown and the blocking behaviour under
write_lock_bh()?

>  	if (nb->node.next) {
>  		list_del(&nb->node);
>  		x25_neigh_put(nb);

Also, the two new operations sit before the if (nb->node.next) guard, so on
the "it was not there" path documented in the comment the function drops no
reference but still kills the T20 timer permanently and discards queued
frames. Both current callers iterate x25_neigh_list, so that branch is
always taken today; would moving the two steps inside the guard keep the
helper consistent with its documented contract for any future caller?