Re: [PATCH V4] netrom: Preventing the use of abnormal neighbor

[Lizhi Xu <lizhi.xu@windriver.com>]

On Wed, 29 Oct 2025 10:59:04 +0800, Lizhi Xu wrote:
> > > The root cause of the problem is that multiple different tasks initiate
> > > SIOCADDRT & NETROM_NODE commands to add new routes, there is no lock
> > > between them to protect the same nr_neigh.
> > >
> > > Task0 can add the nr_neigh.refcount value of 1 on Task1 to routes[2].
> > > When Task2 executes nr_neigh_put(nr_node->routes[2].neighbour), it will
> > > release the neighbour because its refcount value is 1.
> > >
> > > In this case, the following situation causes a UAF on Task2:
> > >
> > > Task0					Task1						Task2
> > > =====					=====						=====
> > > nr_add_node()
> > > nr_neigh_get_dev()			nr_add_node()
> > > 					nr_node_lock()
> > > 					nr_node->routes[2].neighbour->count--
> > > 					nr_neigh_put(nr_node->routes[2].neighbour);
> > > 					nr_remove_neigh(nr_node->routes[2].neighbour)
> > > 					nr_node_unlock()
> > > nr_node_lock()
> > > nr_node->routes[2].neighbour = nr_neigh
> > > nr_neigh_hold(nr_neigh);								nr_add_node()
> > > 											nr_neigh_put()
> > > 											if (nr_node->routes[2].neighbour->count
> > > Description of the UAF triggering process:
> > > First, Task 0 executes nr_neigh_get_dev() to set neighbor refcount to 3.
> > > Then, Task 1 puts the same neighbor from its routes[2] and executes
> > > nr_remove_neigh() because the count is 0. After these two operations,
> > > the neighbor's refcount becomes 1. Then, Task 0 acquires the nr node
> > > lock and writes it to its routes[2].neighbour.
> > > Finally, Task 2 executes nr_neigh_put(nr_node->routes[2].neighbour) to
> > > release the neighbor. The subsequent execution of the neighbor->count
> > > check triggers a UAF.
> > 
> > I looked at the code quite a bit and I think this could possibly avoid
> > the above mentioned race, but this whole area looks quite confusing to me.
> > 
> > I think it would be helpful if you could better describe the relevant
> > scenario starting from the initial setup (no nodes, no neighs).
> OK. Let me fill in the origin of neigh.
> 
> Task3
> =====
> nr_add_node()
> [146]if ((nr_neigh = kmalloc(sizeof(*nr_neigh), GFP_ATOMIC)) == NULL)
> [253]nr_node->routes[2].neighbour = nr_neigh;
> [255]nr_neigh_hold(nr_neigh);
> [256]nr_neigh->count++;
> 
> neigh is created on line 146 in nr_add_node(), and added to node on
> lines 253-256. It occurs before all Task0, Task1, and Task2.
> 
> Note:
> 1. [x], x is line number.
> 2. During my debugging process, I didn't pay attention to where the node
> was created, and I apologize that I cannot provide the relevant creation
> process.
Hi everyone, 
Today is my last day at WindRiver. Starting tomorrow, my email address
lizhi.xu@windriver.com will no longer be used;
I will use eadavis@qq.com thereafter.

BR,
Lizhi