netdev - [PATCH] xfrm: policy: Restructure RCU-read locking in xfrm_sk_policy

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: <20210618141101.18168-1-varad.gautam@suse.com>
Date:   Fri, 18 Jun 2021 16:11:01 +0200
From:   Varad Gautam <varad.gautam@...e.com>
To:     linux-kernel@...r.kernel.org
Cc:     Varad Gautam <varad.gautam@...e.com>,
        linux-rt-users <linux-rt-users@...r.kernel.org>,
        netdev@...r.kernel.org,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Florian Westphal <fw@...len.de>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        Frederic Weisbecker <frederic@...nel.org>,
        stable@...r.kernel.org
Subject: [PATCH] xfrm: policy: Restructure RCU-read locking in xfrm_sk_policy_lookup

Commit "xfrm: policy: Read seqcount outside of rcu-read side in
xfrm_policy_lookup_bytype" [Linked] resolved a locking bug in
xfrm_policy_lookup_bytype that causes an RCU reader-writer deadlock on
the mutex wrapped by xfrm_policy_hash_generation on PREEMPT_RT since
77cc278f7b20 ("xfrm: policy: Use sequence counters with associated
lock").

However, xfrm_sk_policy_lookup can still reach xfrm_policy_lookup_bytype
while holding rcu_read_lock(), as:
xfrm_sk_policy_lookup()
  rcu_read_lock()
  security_xfrm_policy_lookup()
    xfrm_policy_lookup()
      xfrm_policy_lookup_bytype()
        read_seqcount_begin()
          mutex_lock(&hash_resize_mutex)

This results in the same deadlock on hash_resize_mutex, where xfrm_hash_resize
is holding the mutex and sleeping inside synchronize_rcu, and
xfrm_policy_lookup_bytype blocks on the mutex inside the RCU read side
critical section.

To resolve this, shorten the RCU read side critical section within
xfrm_sk_policy_lookup by taking a reference on the associated xfrm_policy,
and avoid calling xfrm_policy_lookup_bytype with the rcu_read_lock() held.

Fixes: 77cc278f7b20 ("xfrm: policy: Use sequence counters with associated lock")
Link: https://lore.kernel.org/r/20210528160407.32127-1-varad.gautam@suse.com/
Signed-off-by: Varad Gautam <varad.gautam@...e.com>
Cc: stable@...r.kernel.org # v5.9
---
 net/xfrm/xfrm_policy.c | 65 +++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 0b7409f19ecb1..28e072007c72d 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2152,42 +2152,47 @@ static struct xfrm_policy *xfrm_sk_policy_lookup(const struct sock *sk, int dir,
 						 u16 family, u32 if_id)
 {
 	struct xfrm_policy *pol;
+	bool match;
+	int err = 0;
 
-	rcu_read_lock();
  again:
+	rcu_read_lock();
 	pol = rcu_dereference(sk->sk_policy[dir]);
-	if (pol != NULL) {
-		bool match;
-		int err = 0;
-
-		if (pol->family != family) {
-			pol = NULL;
-			goto out;
-		}
+	if (pol == NULL) {
+		rcu_read_unlock();
+		goto out;
+	}
 
-		match = xfrm_selector_match(&pol->selector, fl, family);
-		if (match) {
-			if ((sk->sk_mark & pol->mark.m) != pol->mark.v ||
-			    pol->if_id != if_id) {
-				pol = NULL;
-				goto out;
-			}
-			err = security_xfrm_policy_lookup(pol->security,
-						      fl->flowi_secid,
-						      dir);
-			if (!err) {
-				if (!xfrm_pol_hold_rcu(pol))
-					goto again;
-			} else if (err == -ESRCH) {
-				pol = NULL;
-			} else {
-				pol = ERR_PTR(err);
-			}
-		} else
-			pol = NULL;
+	/* Take a reference on this pol and call rcu_read_unlock(). */
+	if (!xfrm_pol_hold_rcu(pol)) {
+		rcu_read_unlock();
+		goto again;
 	}
-out:
 	rcu_read_unlock();
+
+	if (pol->family != family)
+		goto out_put;
+
+	match = xfrm_selector_match(&pol->selector, fl, family);
+	if (!match)
+		goto out_put;
+
+	if ((sk->sk_mark & pol->mark.m) != pol->mark.v ||
+	    pol->if_id != if_id)
+		goto out_put;
+
+	err = security_xfrm_policy_lookup(pol->security,
+					fl->flowi_secid,
+					dir);
+	if (!err) {
+		/* Safe to return, we have a ref on pol. */
+		goto out;
+	}
+
+ out_put:
+	xfrm_pol_put(pol);
+	pol = (err && err != -ESRCH) ? ERR_PTR(err) : NULL;
+ out:
 	return pol;
 }
 
-- 
2.30.2