net/netrom/nr_route.c | 4 ++++ 1 file changed, 4 insertions(+)
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.
The solution to the problem is to use a lock to synchronize each add a
route to node, but for rigor, I'll add locks to related ioctl and route
frame operations to maintain synchronization.
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
Call Trace:
<TASK>
nr_add_node+0x25db/0x2c00 net/netrom/nr_route.c:248
Reported-by: syzbot+2860e75836a08b172755@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2860e75836a08b172755
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: update comments for cause uaf
V2 -> V3: sync neighbor operations in ioctl and route frame, update comments
net/netrom/nr_route.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index b94cb2ffbaf8..debe3e925338 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -40,6 +40,7 @@ static HLIST_HEAD(nr_node_list);
static DEFINE_SPINLOCK(nr_node_list_lock);
static HLIST_HEAD(nr_neigh_list);
static DEFINE_SPINLOCK(nr_neigh_list_lock);
+static DEFINE_MUTEX(neighbor_lock);
static struct nr_node *nr_node_get(ax25_address *callsign)
{
@@ -633,6 +634,8 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
ax25_digi digi;
int ret;
+ guard(mutex)(&neighbor_lock);
+
switch (cmd) {
case SIOCADDRT:
if (copy_from_user(&nr_route, arg, sizeof(struct nr_route_struct)))
@@ -765,6 +768,7 @@ int nr_route_frame(struct sk_buff *skb, ax25_cb *ax25)
nr_dest = (ax25_address *)(skb->data + 7);
if (ax25 != NULL) {
+ guard(mutex)(&neighbor_lock);
ret = nr_add_node(nr_src, "", &ax25->dest_addr, ax25->digipeat,
ax25->ax25_dev->dev, 0,
READ_ONCE(sysctl_netrom_obsolescence_count_initialiser));
--
2.43.0On 10/21/25 10:35 AM, 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.
>
> The solution to the problem is to use a lock to synchronize each add a
> route to node, but for rigor, I'll add locks to related ioctl and route
> frame operations to maintain synchronization.
I think that adding another locking mechanism on top of an already
complex and not well understood locking and reference infra is not the
right direction.
Why reordering the statements as:
if (nr_node->routes[2].neighbour->count == 0 &&
!nr_node->routes[2].neighbour->locked)
nr_remove_neigh(nr_node->routes[2].neighbour);
nr_neigh_put(nr_node->routes[2].neighbour);
is not enough?
> 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
>
> Call Trace:
> <TASK>
> nr_add_node+0x25db/0x2c00 net/netrom/nr_route.c:248
>
> Reported-by: syzbot+2860e75836a08b172755@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=2860e75836a08b172755
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
> V1 -> V2: update comments for cause uaf
> V2 -> V3: sync neighbor operations in ioctl and route frame, update comments
>
> net/netrom/nr_route.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> index b94cb2ffbaf8..debe3e925338 100644
> --- a/net/netrom/nr_route.c
> +++ b/net/netrom/nr_route.c
> @@ -40,6 +40,7 @@ static HLIST_HEAD(nr_node_list);
> static DEFINE_SPINLOCK(nr_node_list_lock);
> static HLIST_HEAD(nr_neigh_list);
> static DEFINE_SPINLOCK(nr_neigh_list_lock);
> +static DEFINE_MUTEX(neighbor_lock);
>
> static struct nr_node *nr_node_get(ax25_address *callsign)
> {
> @@ -633,6 +634,8 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
> ax25_digi digi;
> int ret;
>
> + guard(mutex)(&neighbor_lock);
See:
https://elixir.bootlin.com/linux/v6.18-rc1/source/Documentation/process/maintainer-netdev.rst#L395
/P
On Thu, Oct 23, 2025 at 01:44:18PM +0200, Paolo Abeni wrote:
> Why reordering the statements as:
>
> if (nr_node->routes[2].neighbour->count == 0 &&
> !nr_node->routes[2].neighbour->locked)
> nr_remove_neigh(nr_node->routes[2].neighbour);
> nr_neigh_put(nr_node->routes[2].neighbour);
>
> is not enough?
There are so many unfortunate things like this:
net/netrom/nr_route.c
243 /* It must be better than the worst */
244 if (quality > nr_node->routes[2].quality) {
245 nr_node->routes[2].neighbour->count--;
++/-- are not atomic.
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;
This line should come after the next two lines.
254
255 nr_neigh_hold(nr_neigh);
256 nr_neigh->count++;
257 }
regards,
dan carpenter
On Thu, 23 Oct 2025 13:44:18 +0200, Paolo Abeni 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.
> >
> > The solution to the problem is to use a lock to synchronize each add a
> > route to node, but for rigor, I'll add locks to related ioctl and route
> > frame operations to maintain synchronization.
>
> I think that adding another locking mechanism on top of an already
> complex and not well understood locking and reference infra is not the
> right direction.
>
> Why reordering the statements as:
>
> if (nr_node->routes[2].neighbour->count == 0 &&
> !nr_node->routes[2].neighbour->locked)
> nr_remove_neigh(nr_node->routes[2].neighbour);
> nr_neigh_put(nr_node->routes[2].neighbour);
>
> is not enough?
This is not enough, the same uaf will appear, nr_remove_neigh() will also
execute nr_neigh_put(), and then executing nr_neigh_put() again will
trigger the uaf.
>
> > 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
> >
> > Call Trace:
> > <TASK>
> > nr_add_node+0x25db/0x2c00 net/netrom/nr_route.c:248
> >
> > Reported-by: syzbot+2860e75836a08b172755@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=2860e75836a08b172755
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
>
>
>
> > ---
> > V1 -> V2: update comments for cause uaf
> > V2 -> V3: sync neighbor operations in ioctl and route frame, update comments
> >
> > net/netrom/nr_route.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> > index b94cb2ffbaf8..debe3e925338 100644
> > --- a/net/netrom/nr_route.c
> > +++ b/net/netrom/nr_route.c
> > @@ -40,6 +40,7 @@ static HLIST_HEAD(nr_node_list);
> > static DEFINE_SPINLOCK(nr_node_list_lock);
> > static HLIST_HEAD(nr_neigh_list);
> > static DEFINE_SPINLOCK(nr_neigh_list_lock);
> > +static DEFINE_MUTEX(neighbor_lock);
> >
> > static struct nr_node *nr_node_get(ax25_address *callsign)
> > {
> > @@ -633,6 +634,8 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
> > ax25_digi digi;
> > int ret;
> >
> > + guard(mutex)(&neighbor_lock);
>
> See:
>
> https://elixir.bootlin.com/linux/v6.18-rc1/source/Documentation/process/maintainer-netdev.rst#L395
Using guard is not recommended. I'll reconsider.
BR,
Lizhi
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.
Filter out neighbors with a refcount of 1 to avoid unsafe conditions.
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
Call Trace:
<TASK>
nr_add_node+0x25db/0x2c00 net/netrom/nr_route.c:248
Reported-by: syzbot+2860e75836a08b172755@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=2860e75836a08b172755
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
V1 -> V2: update comments for cause uaf
V2 -> V3: sync neighbor operations in ioctl and route frame, update comments
V3 -> V4: Preventing the use of neighbors with a reference count of 1
net/netrom/nr_route.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
index b94cb2ffbaf8..1ef2743a5ec0 100644
--- a/net/netrom/nr_route.c
+++ b/net/netrom/nr_route.c
@@ -100,7 +100,7 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
{
struct nr_node *nr_node;
struct nr_neigh *nr_neigh;
- int i, found;
+ int i, found, ret = 0;
struct net_device *odev;
if ((odev=nr_dev_get(nr)) != NULL) { /* Can't add routes to ourself */
@@ -212,6 +212,10 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
return 0;
}
nr_node_lock(nr_node);
+ if (refcount_read(&nr_neigh->refcount) == 1) {
+ ret = -EINVAL;
+ goto out;
+ }
if (quality != 0)
strscpy(nr_node->mnemonic, mnemonic);
@@ -279,10 +283,11 @@ static int __must_check nr_add_node(ax25_address *nr, const char *mnemonic,
}
}
+out:
nr_neigh_put(nr_neigh);
nr_node_unlock(nr_node);
nr_node_put(nr_node);
- return 0;
+ return ret;
}
static void nr_remove_node_locked(struct nr_node *nr_node)
--
2.43.0On 10/23/25 3:50 PM, 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). Thanks, Paolo
On Tue, 28 Oct 2025 15:13:37 +0100, Paolo Abeni 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. BR, Lizhi
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
On Thu, Oct 23, 2025 at 4:44 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 10/21/25 10:35 AM, 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.
> >
> > The solution to the problem is to use a lock to synchronize each add a
> > route to node, but for rigor, I'll add locks to related ioctl and route
> > frame operations to maintain synchronization.
>
> I think that adding another locking mechanism on top of an already
> complex and not well understood locking and reference infra is not the
> right direction.
>
> Why reordering the statements as:
>
> if (nr_node->routes[2].neighbour->count == 0 &&
> !nr_node->routes[2].neighbour->locked)
> nr_remove_neigh(nr_node->routes[2].neighbour);
> nr_neigh_put(nr_node->routes[2].neighbour);
>
> is not enough?
>
> > 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
> >
> > Call Trace:
> > <TASK>
> > nr_add_node+0x25db/0x2c00 net/netrom/nr_route.c:248
> >
> > Reported-by: syzbot+2860e75836a08b172755@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=2860e75836a08b172755
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
>
>
>
> > ---
> > V1 -> V2: update comments for cause uaf
> > V2 -> V3: sync neighbor operations in ioctl and route frame, update comments
> >
> > net/netrom/nr_route.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/net/netrom/nr_route.c b/net/netrom/nr_route.c
> > index b94cb2ffbaf8..debe3e925338 100644
> > --- a/net/netrom/nr_route.c
> > +++ b/net/netrom/nr_route.c
> > @@ -40,6 +40,7 @@ static HLIST_HEAD(nr_node_list);
> > static DEFINE_SPINLOCK(nr_node_list_lock);
> > static HLIST_HEAD(nr_neigh_list);
> > static DEFINE_SPINLOCK(nr_neigh_list_lock);
> > +static DEFINE_MUTEX(neighbor_lock);
> >
> > static struct nr_node *nr_node_get(ax25_address *callsign)
> > {
> > @@ -633,6 +634,8 @@ int nr_rt_ioctl(unsigned int cmd, void __user *arg)
> > ax25_digi digi;
> > int ret;
> >
> > + guard(mutex)(&neighbor_lock);
>
> See:
>
> https://elixir.bootlin.com/linux/v6.18-rc1/source/Documentation/process/maintainer-netdev.rst#L395
>
I would also try to use a single spinlock : ie fuse together
nr_node_list_lock and nr_neigh_list_lock
Having two locks for something that is primarily used by fuzzers
nowadays is wasting our time.
© 2016 - 2026 Red Hat, Inc.