netdev - Re: [PATCH net] net: ipv4: fix one memleak in __inet_del

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: <734181f9-b340-daa2-0772-7ed220eee150@ssi.bg>
Date: Wed, 6 Sep 2023 13:30:18 +0300 (EEST)
From: Julian Anastasov <ja@....bg>
To: "liujian (CE)" <liujian56@...wei.com>
cc: davem@...emloft.net, dsahern@...nel.org, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, hadi@...erus.ca,
        netdev@...r.kernel.org
Subject: Re: [PATCH net] net: ipv4: fix one memleak in __inet_del_ifa()


	Hello,

On Wed, 6 Sep 2023, liujian (CE) wrote:

> On 2023/9/6 1:20, Julian Anastasov wrote:
> > 
> > On Tue, 5 Sep 2023, Liu Jian wrote:
> > 
> >> I got the below warning when do fuzzing test:
> >> unregister_netdevice: waiting for bond0 to become free. Usage count = 2
> >>
> >> It can be repoduced via:
> >>
> >> ip link add bond0 type bond
> >> sysctl -w net.ipv4.conf.bond0.promote_secondaries=1
> >> ip addr add 4.117.174.103/0 scope 0x40 dev bond0
> >> ip addr add 192.168.100.111/255.255.255.254 scope 0 dev bond0
> >> ip addr add 0.0.0.4/0 scope 0x40 secondary dev bond0
> >> ip addr del 4.117.174.103/0 scope 0x40 dev bond0
> >> ip link delete bond0 type bond
> >>
> >> In this reproduction test case, an incorrect 'last_prim' is found in
> >> __inet_del_ifa(), as a result, the secondary address(0.0.0.4/0 scope 0x40)
> >> is lost. The memory of the secondary address is leaked and the reference of
> >> in_device and net_device is leaked.

	We can also explain that the problem occurs when we delete
the first primary address and the promoted address is leaked because
it is attached to the to-be-freed primary address instead of to ifa_list.

> >> Fix this problem by modifying the PROMOTE_SECONDANCE behavior as follows:
> >> 1. Traverse in_dev->ifa_list to search for the actual 'last_prim'.
> >> 2. When last_prim is empty, move 'promote' to the in_dev->ifa_list header.
> > 
> > 	So, the problem is that last_prim initially points to the
> > first primary address that we are actually removing. Looks like with
> > last_prim we try to promote the secondary IP after all primaries with
> > scope >= our scope, i.e. simulating a new IP insert. As the secondary IPs
> > have same scope as their primary, why just not remove the last_prim
> > var/code and to insert the promoted secondary at the same place as the
> > deleted primary? May be your patch does the same: insert at same pos?
> > 
> > Before deletion:
> > 1. primary1 scope global (to be deleted)
> > 2. primary2 scope global
> > 3. promoted_secondary
> > 
> > After deletion (old way, promote as a new insertion):
> > 1. primary2 scope global
> > 2. promoted_secondary scope global (inserted as new primary)
> > 
> It is :
> After deletion (old way, promoted_secondary lost):
> 1. primary2 scope global

	Yes, that is what happens :)

> > After deletion (new way, promote at same place):
> > 1. promoted_secondary scope global (now primary, inserted at same place)
> > 2. primary2 scope global
> > 
> >  What I mean is to use ifap as last_prim, not tested:
> > 
> Yes, This is better and it can work also. Thanks.
> Tested-by: Liu Jian <liujian56@...wei.com>

	But let me propose another version. It is a minimal
bugfix that does not change the place where the promoted address
is added and just converts last_prim to be rcu ptr to insert
position. last_prim will start from ifap because the promoted
address can not be added before this position. It is better
not to reorder the IPs because scripts may depend on the
old behavior to add the promoted IP after all others for
the scope. If you find this version better you can post it
as v2 and I'll sign it too.

diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 5deac0517ef7..37be82496322 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -355,14 +355,14 @@ static void __inet_del_ifa(struct in_device *in_dev,
 {
 	struct in_ifaddr *promote = NULL;
 	struct in_ifaddr *ifa, *ifa1;
-	struct in_ifaddr *last_prim;
+	struct in_ifaddr __rcu **last_prim;
 	struct in_ifaddr *prev_prom = NULL;
 	int do_promote = IN_DEV_PROMOTE_SECONDARIES(in_dev);
 
 	ASSERT_RTNL();
 
 	ifa1 = rtnl_dereference(*ifap);
-	last_prim = rtnl_dereference(in_dev->ifa_list);
+	last_prim = ifap;
 	if (in_dev->dead)
 		goto no_promotions;
 
@@ -376,7 +376,7 @@ static void __inet_del_ifa(struct in_device *in_dev,
 		while ((ifa = rtnl_dereference(*ifap1)) != NULL) {
 			if (!(ifa->ifa_flags & IFA_F_SECONDARY) &&
 			    ifa1->ifa_scope <= ifa->ifa_scope)
-				last_prim = ifa;
+				last_prim = &ifa->ifa_next;
 
 			if (!(ifa->ifa_flags & IFA_F_SECONDARY) ||
 			    ifa1->ifa_mask != ifa->ifa_mask ||
@@ -440,9 +440,9 @@ static void __inet_del_ifa(struct in_device *in_dev,
 
 			rcu_assign_pointer(prev_prom->ifa_next, next_sec);
 
-			last_sec = rtnl_dereference(last_prim->ifa_next);
+			last_sec = rtnl_dereference(*last_prim);
 			rcu_assign_pointer(promote->ifa_next, last_sec);
-			rcu_assign_pointer(last_prim->ifa_next, promote);
+			rcu_assign_pointer(*last_prim, promote);
 		}
 
 		promote->ifa_flags &= ~IFA_F_SECONDARY;

Regards

--
Julian Anastasov <ja@....bg>