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

Dan Carpenter
2025-10-20T12:57:45Z

On Mon, Oct 20, 2025 at 03:25:40PM +0300, Dan Carpenter wrote:
> On Mon, Oct 20, 2025 at 07:02:44PM +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 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:
> >
> > 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()
> >
> > The solution to the problem is to use a lock to synchronize each add a route
> > to node.
>
> This chart is still not right. Let me add line numbers to your chart:
>
> netrom/nr_route.c
> 214 nr_node_lock(nr_node);
> 215
> 216 if (quality != 0)
> 217 strscpy(nr_node->mnemonic, mnemonic);
> 218
> 219 for (found = 0, i = 0; i < nr_node->count; i++) {
> 220 if (nr_node->routes[i].neighbour == nr_neigh) {
> 221 nr_node->routes[i].quality = quality;
> 222 nr_node->routes[i].obs_count = obs_count;

Should we call nr_neigh->count++ if we found it? I guess I don't
really understand what nr_neigh->count is counting... It would be
really nice if someone added some comments explaining how the ref
counting worked.

> 223 found = 1;
> 224 break;
> 225 }
> 226 }

regards,
dan carpenter