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

Dan Carpenter
2025-10-20T12:25:49Z

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;
223 found = 1;
224 break;
225 }
226 }
227
228 if (!found) {
229 /* We have space at the bottom, slot it in */
230 if (nr_node->count < 3) {
231 nr_node->routes[2] = nr_node->routes[1];
232 nr_node->routes[1] = nr_node->routes[0];
233
234 nr_node->routes[0].quality = quality;
235 nr_node->routes[0].obs_count = obs_count;
236 nr_node->routes[0].neighbour = nr_neigh;
237
238 nr_node->which++;
239 nr_node->count++;
240 nr_neigh_hold(nr_neigh);
241 nr_neigh->count++;
242 } else {
243 /* It must be better than the worst */
244 if (quality > nr_node->routes[2].quality) {
245 nr_node->routes[2].neighbour->count--;
246 nr_neigh_put(nr_node->routes[2].neighbour);
247
248 if (nr_node->routes[2].neighbour->count == 0 && !nr_node->routes[2].neighbour->locked)
249 nr_remove_neigh(nr_node->routes[2].neighbour);
250
251 nr_node->routes[2].quality = quality;
252 nr_node->routes[2].obs_count = obs_count;
253 nr_node->routes[2].neighbour = nr_neigh;
254
255 nr_neigh_hold(nr_neigh);
256 nr_neigh->count++;
257 }
258 }
259 }

Task0 Task1 Task2
===== ===== =====
[97] nr_add_node()
[113] nr_neigh_get_dev() [97] nr_add_node()
[214] nr_node_lock()
[245] nr_node->routes[2].neighbour->count--
[246] nr_neigh_put(nr_node->routes[2].neighbour);
[248] nr_remove_neigh(nr_node->routes[2].neighbour)
[283] nr_node_unlock()
[214] nr_node_lock()
[253] nr_node->routes[2].neighbour = nr_neigh
[254] nr_neigh_hold(nr_neigh); [97] nr_add_node()
[XXX] nr_neigh_put()
^^^^^^^^^^^^^^^^^^^^

These charts are supposed to be chronological so [XXX] is wrong because the
use after free happens on line [248]. Do we really need three threads to
make this race work?

>
> syzbot reported:
> BUG: KASAN: slab-use-after-free in nr_add_node+0x25db/0x2c00 net/netrom/nr_route.c:248
^^^

> Read of size 4 at addr ffff888051e6e9b0 by task syz.1.2539/8741

I'm sure you tested your patch and that it fixes the bug, but I just
wonder if it's the best possible fix?

regards,
dan carpenter