netfilter-devel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sean Tranchetti <stranche@codeaurora.org>
To: pablo@netfilter.org, netfilter-devel@vger.kernel.org
Cc: Sean Tranchetti <stranche@codeaurora.org>
Subject: [PATCH nf] Revert "netfilter: unlock xt_table earlier in __do_replace"
Date: Wed, 22 Jan 2020 17:47:35 -0700	[thread overview]
Message-ID: <1579740455-17249-1-git-send-email-stranche@codeaurora.org> (raw)

A recently reported crash in the x_tables framework seems to stem from
a potential race condition between adding rules to a table and having a
packet traversing the table at the same time.

In the crash, the jumpstack being used by the table traversal was freed
by the table replace code. After performing some bisection, it seems that
commit f31e5f1a891f ("netfilter: unlock xt_table earlier in __do_replace")
exposed this race condition by unlocking the table before the
get_old_counters() routine was called to perform the synchronization.

Call Stack:
	Unable to handle kernel paging request at virtual address
	006b6b6b6b6b6bc5

	pc : ipt_do_table+0x3b8/0x660
	lr : ipt_do_table+0x31c/0x660
	Call trace:
	ipt_do_table+0x3b8/0x660
	iptable_mangle_hook+0x58/0xf8
	nf_hook_slow+0x48/0xd8
	__ip_local_out+0xf4/0x138
	__ip_queue_xmit+0x348/0x3a0
	ip_queue_xmit+0x10/0x18

Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
---
 net/ipv4/netfilter/arp_tables.c | 3 +--
 net/ipv4/netfilter/ip_tables.c  | 3 +--
 net/ipv6/netfilter/ip6_tables.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index f1f78a7..85cb189 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -921,8 +921,6 @@ static int __do_replace(struct net *net, const char *name,
 	    (newinfo->number <= oldinfo->initial_entries))
 		module_put(t->me);
 
-	xt_table_unlock(t);
-
 	get_old_counters(oldinfo, counters);
 
 	/* Decrease module usage counts and free resource */
@@ -937,6 +935,7 @@ static int __do_replace(struct net *net, const char *name,
 		net_warn_ratelimited("arptables: counters copy to user failed while replacing table\n");
 	}
 	vfree(counters);
+	xt_table_unlock(t);
 	return ret;
 
  put_module:
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 10b91eb..9f98bc5 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1076,8 +1076,6 @@ static int get_info(struct net *net, void __user *user,
 	    (newinfo->number <= oldinfo->initial_entries))
 		module_put(t->me);
 
-	xt_table_unlock(t);
-
 	get_old_counters(oldinfo, counters);
 
 	/* Decrease module usage counts and free resource */
@@ -1091,6 +1089,7 @@ static int get_info(struct net *net, void __user *user,
 		net_warn_ratelimited("iptables: counters copy to user failed while replacing table\n");
 	}
 	vfree(counters);
+	xt_table_unlock(t);
 	return ret;
 
  put_module:
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index c973ace..f2637bfb 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1093,8 +1093,6 @@ static int get_info(struct net *net, void __user *user,
 	    (newinfo->number <= oldinfo->initial_entries))
 		module_put(t->me);
 
-	xt_table_unlock(t);
-
 	get_old_counters(oldinfo, counters);
 
 	/* Decrease module usage counts and free resource */
@@ -1108,6 +1106,7 @@ static int get_info(struct net *net, void __user *user,
 		net_warn_ratelimited("ip6tables: counters copy to user failed while replacing table\n");
 	}
 	vfree(counters);
+	xt_table_unlock(t);
 	return ret;
 
  put_module:
-- 
1.9.1


             reply	other threads:[~2020-01-23  0:48 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-23  0:47 Sean Tranchetti [this message]
2020-01-23 10:29 ` [PATCH nf] Revert "netfilter: unlock xt_table earlier in __do_replace" Florian Westphal
2020-01-23 20:08   ` stranche
2020-02-03 10:51   ` Xin Long

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1579740455-17249-1-git-send-email-stranche@codeaurora.org \
    --to=stranche@codeaurora.org \
    --cc=netfilter-devel@vger.kernel.org \
    --cc=pablo@netfilter.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).