netdev - Re: [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-prev] [day] [month] [year] [list]

Message-ID: <de514556-a86b-4bb4-b317-a3b29188e6e3@kernel.org>
Date: Sat, 6 Apr 2024 13:51:25 -0600
From: David Ahern <dsahern@...nel.org>
To: Jiri Benc <jbenc@...hat.com>, netdev@...r.kernel.org
Cc: Stephen Hemminger <stephen@...workplumber.org>
Subject: Re: [PATCH net] ipv6: fix race condition between ipv6_get_ifaddr and
 ipv6_del_addr

On 4/5/24 4:54 AM, Jiri Benc wrote:
> 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(-)
> 


Reviewed-by: David Ahern <dsahern@...nel.org>