netdev - [PATCH net] ipv6: fix race condition between ipv6_get_ifaddr and ipv6_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-next>] [day] [month] [year] [list]

Message-ID: <8bbe1218656e66552ff28cbee8c7d1f0ffd8e9fd.1712314149.git.jbenc@redhat.com>
Date: Fri,  5 Apr 2024 12:54:05 +0200
From: Jiri Benc <jbenc@...hat.com>
To: netdev@...r.kernel.org
Cc: Stephen Hemminger <stephen@...workplumber.org>,
	David Ahern <dsahern@...nel.org>
Subject: [PATCH net] ipv6: fix race condition between ipv6_get_ifaddr and ipv6_del_addr

Although ipv6_get_ifaddr walks inet6_addr_lst under the RCU lock, it
still means hlist_for_each_entry_rcu can return an item that got removed
from the list. The memory itself of such item is not freed thanks to RCU
but nothing guarantees the actual content of the memory is sane.

In particular, the reference count can be zero. This can happen if
ipv6_del_addr is called in parallel. ipv6_del_addr removes the entry
from inet6_addr_lst (hlist_del_init_rcu(&ifp->addr_lst)) and drops all
references (__in6_ifa_put(ifp) + in6_ifa_put(ifp)). With bad enough
timing, this can happen:

1. In ipv6_get_ifaddr, hlist_for_each_entry_rcu returns an entry.

2. Then, the whole ipv6_del_addr is executed for the given entry. The
   reference count drops to zero and kfree_rcu is scheduled.

3. ipv6_get_ifaddr continues and increments the reference count
   (in6_ifa_hold).

4. The rcu is unlocked and the entry is freed.

5. Later, the reference count is dropped to zero (again) and kfree_rcu
   is scheduled (again).

Prevent increasing of the reference count in such case. The name
in6_ifa_hold_safe is chosen to mimic the existing fib6_info_hold_safe.

Fixes: 5c578aedcb21d ("IPv6: convert addrconf hash list to RCU")
Signed-off-by: Jiri Benc <jbenc@...hat.com>
---

Side note: While this fixes one bug, there may be more locking bugs
lurking aroung inet6_ifaddr. The semantics of locking of inet6_ifaddr is
wild and fragile. Some of the fields are freed in ipv6_del_addr and
guarded by ifa->state == INET6_IFADDR_STATE_DEAD and RTNL. Some of the
fields are freed in inet6_ifa_finish_destroy and guarded by ifa->refcnt
and RCU. Needless to say, this semantics is undocumented. Worse,
ifa->state guard may not be enough. For example, ipv6_get_ifaddr can
still return an entry that proceeded through ipv6_del_addr, which means
ifa->state is INET6_IFADDR_STATE_DEAD. However, at least some callers
(e.g. ndisc_recv_ns) seem to change ifa->state to something else. As
another example, ipv6_del_addr relies on ifa->flags, which are changed
throughout the code without RTNL. All of this may be okay but it's far
from clear.
---
 include/net/addrconf.h | 4 ++++
 net/ipv6/addrconf.c    | 7 ++++---
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/net/addrconf.h b/include/net/addrconf.h
index 9d06eb945509..62a407db1bf5 100644
--- a/include/net/addrconf.h
+++ b/include/net/addrconf.h
@@ -438,6 +438,10 @@ static inline void in6_ifa_hold(struct inet6_ifaddr *ifp)
 	refcount_inc(&ifp->refcnt);
 }

+static inline bool in6_ifa_hold_safe(struct inet6_ifaddr *ifp)
+{
+	return refcount_inc_not_zero(&ifp->refcnt);
+}

 /*
  *	compute link-local solicited-node multicast address
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 92db9b474f2b..779aa6ecdd49 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2091,9 +2091,10 @@ struct inet6_ifaddr *ipv6_get_ifaddr(struct net *net, const struct in6_addr *add
 		if (ipv6_addr_equal(&ifp->addr, addr)) {
 			if (!dev || ifp->idev->dev == dev ||
 			    !(ifp->scope&(IFA_LINK|IFA_HOST) || strict)) {
-				result = ifp;
-				in6_ifa_hold(ifp);
-				break;
+				if (in6_ifa_hold_safe(ifp)) {
+					result = ifp;
+					break;
+				}
 			}
 		}
 	}
-- 
2.42.0