netdev - Re: [PATCH] netrom: Prevent race conditions between multiple add route

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aPYKgFTIroUhJAJA@stanley.mountain>
Date: Mon, 20 Oct 2025 13:10:08 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: syzbot+2860e75836a08b172755@...kaller.appspotmail.com,
	davem@...emloft.net, edumazet@...gle.com, horms@...nel.org,
	kuba@...nel.org, linux-hams@...r.kernel.org,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	pabeni@...hat.com, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] netrom: Prevent race conditions between multiple add
 route

On Mon, Oct 20, 2025 at 04:13:59PM +0800, Lizhi Xu wrote:
> The root cause of the problem is that multiple different tasks initiate
> NETROM_NODE commands to add new routes, there is no lock between them to
> protect the same nr_neigh.
> Task0 may add the nr_neigh.refcount value of 1 on Task1 to routes[2].
> When Task3 executes nr_neigh_put(nr_node->routes[2].neighbour), it will

s/Task3/Task1/

> release the neighbour because its refcount value is 1.
> 

The refcount would be 2 and then drop to zero.  Both nr_neigh_put() and
nr_remove_neigh() drop the refcount.

> In this case, the following situation causes a UAF:
> 
> Task0					Task1
> =====					=====
> nr_add_node()
> nr_neigh_get_dev()			nr_add_node()
> 					nr_node->routes[2].neighbour->count--

Does this line really matter in terms of the use after free?

> 					nr_neigh_put(nr_node->routes[2].neighbour);
> 					nr_remove_neigh(nr_node->routes[2].neighbour)
> nr_node->routes[2].neighbour = nr_neigh
> nr_neigh_hold(nr_neigh);


This chart is confusing.  It says that that the nr_neigh_hold() is the use
after free.  But we called nr_remove_neigh(nr_node->routes[2].neighbour)
before we assigned nr_node->routes[2].neighbour = nr_neigh...

The sysbot report says that the free happens on:

	r_neigh_put(nr_node->routes[2].neighbour);

and the use after free happens on the next line:

	if (nr_node->routes[2].neighbour->count == 0 && !nr_node->routes[2].neighbour->locked)

Which does suggest that somewhere the refcount is 1 when it should be
at least 2...  It could be that two threads call nr_neigh_put() at
basically the same time, but that doesn't make sense either because
we're holding the nr_node_lock(nr_node)...

regards,
dan carpenter