netdev - [RFC] ifa_list needs proper rcu protection

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: <7bdc26e6-ce41-02ba-baef-3e4e908f6dd7@gmail.com>
Date:   Thu, 25 Apr 2019 15:50:53 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Networking <netdev@...r.kernel.org>
Cc:     David Ahern <dsahern@...il.com>, Florian Westphal <fw@...len.de>
Subject: [RFC] ifa_list needs proper rcu protection

It looks that unless RTNL is held, accessing ifa_list needs proper RCU protection ?

indev->ifa_list can be changed under us by another cpu (which owns RTNL)

Lets took an example.

(A proper rcu_dereference() with an happy sparse support would require adding __rcu attribute,
 I put a READ_ONCE() which should be just fine in this particular context)

diff --git a/net/netfilter/nf_nat_redirect.c b/net/netfilter/nf_nat_redirect.c
index 78a9e6454ff3d712926397beb904b478b8fab0f1..8619b8d02b0530c5735c31d029f1d79969d979c7 100644
--- a/net/netfilter/nf_nat_redirect.c
+++ b/net/netfilter/nf_nat_redirect.c
@@ -48,14 +48,17 @@ nf_nat_redirect_ipv4(struct sk_buff *skb,
                newdst = htonl(0x7F000001);
        } else {
                struct in_device *indev;
-               struct in_ifaddr *ifa;
 
                newdst = 0;
 
                indev = __in_dev_get_rcu(skb->dev);
-               if (indev && indev->ifa_list) {
-                       ifa = indev->ifa_list;
-                       newdst = ifa->ifa_local;
+               if (indev) {
+                       struct in_ifaddr *ifa;
+
+                       ifa = READ_ONCE(indev->ifa_list); // rcu_dereference(xxx)
+
+                       if (ifa)
+                               newdst = ifa->ifa_local;
                }
 
                if (!newdst)