linux-kernel - Re: [PATCH V2] 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: <aPYsDVBMUQ0X_ulN@stanley.mountain>
Date: Mon, 20 Oct 2025 15:33:17 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Lizhi Xu <lizhi.xu@...driver.com>
Cc: 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,
	syzbot+2860e75836a08b172755@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V2] netrom: Prevent race conditions between multiple add
 route

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