History

netfilter: x_tables: Switch synchronization to RCU

When running concurrent iptables rules replacement with data, the per CPU sequence count is checked after the assignment of the new information. The sequence count is used to synchronize with the packet path without the use of any explicit locking. If there are any packets in the packet path using the table information, the sequence count is incremented to an odd value and is incremented to an even after the packet process completion. The new table value assignment is followed by a write memory barrier so every CPU should see the latest value. If the packet path has started with the old table information, the sequence counter will be odd and the iptables replacement will wait till the sequence count is even prior to freeing the old table info. However, this assumes that the new table information assignment and the memory barrier is actually executed prior to the counter check in the replacement thread. If CPU decides to execute the assignment later as there is no user of the table information prior to the sequence check, the packet path in another CPU may use the old table information. The replacement thread would then free the table information under it leading to a use after free in the packet processing context- Unable to handle kernel NULL pointer dereference at virtual address 000000000000008e pc : ip6t_do_table+0x5d0/0x89c lr : ip6t_do_table+0x5b8/0x89c ip6t_do_table+0x5d0/0x89c ip6table_filter_hook+0x24/0x30 nf_hook_slow+0x84/0x120 ip6_input+0x74/0xe0 ip6_rcv_finish+0x7c/0x128 ipv6_rcv+0xac/0xe4 __netif_receive_skb+0x84/0x17c process_backlog+0x15c/0x1b8 napi_poll+0x88/0x284 net_rx_action+0xbc/0x23c __do_softirq+0x20c/0x48c This could be fixed by forcing instruction order after the new table information assignment or by switching to RCU for the synchronization.

Subash Abhinov Kasiviswanathan committed 5 years ago

Commit cc00bca1 parent 819f56b5 files · +40 −56Browse files

Unified · Split

5 files
  1. include/linux/netfilter/x_tables.h +4 −1
  2. net/ipv4/netfilter/arp_tables.c +7 −7
  3. net/ipv4/netfilter/ip_tables.c +7 −7
  4. net/ipv6/netfilter/ip6_tables.c +7 −7
  5. net/netfilter/x_tables.c +15 −34
include/linux/netfilter/x_tables.h+4−1
@@ -227,7 +227,7 @@ struct xt_table { · 226 unmodified lines
227 unsigned int valid_hooks;227 unsigned int valid_hooks;
228 228
229 /* Man behind the curtain... */229 /* Man behind the curtain... */
230 struct xt_table_info *private;230 struct xt_table_info __rcu *private;
231 231
232 /* Set this to THIS_MODULE if you are a module, otherwise NULL */232 /* Set this to THIS_MODULE if you are a module, otherwise NULL */
233 struct module *me;233 struct module *me;
@@ -448,6 +448,9 @@ xt_get_per_cpu_counter(struct xt_counters *cnt, unsigned int cpu) · 214 unmodified lines
448 448
449struct nf_hook_ops *xt_hook_ops_alloc(const struct xt_table *, nf_hookfn *);449struct nf_hook_ops *xt_hook_ops_alloc(const struct xt_table *, nf_hookfn *);
450 450
451struct xt_table_info
452*xt_table_get_private_protected(const struct xt_table *table);
453
451#ifdef CONFIG_COMPAT454#ifdef CONFIG_COMPAT
452#include <net/compat.h>455#include <net/compat.h>
453 456
net/ipv4/netfilter/arp_tables.c+7−7
@@ -203,7 +203,7 @@ unsigned int arpt_do_table(struct sk_buff *skb, · 202 unmodified lines
203 203
204 local_bh_disable();204 local_bh_disable();
205 addend = xt_write_recseq_begin();205 addend = xt_write_recseq_begin();
206 private = READ_ONCE(table->private); /* Address dependency. */206 private = rcu_access_pointer(table->private);
207 cpu = smp_processor_id();207 cpu = smp_processor_id();
208 table_base = private->entries;208 table_base = private->entries;
209 jumpstack = (struct arpt_entry **)private->jumpstack[cpu];209 jumpstack = (struct arpt_entry **)private->jumpstack[cpu];
@@ -649,7 +649,7 @@ static struct xt_counters *alloc_counters(const struct xt_table *table) · 439 unmodified lines
649{649{
650 unsigned int countersize;650 unsigned int countersize;
651 struct xt_counters *counters;651 struct xt_counters *counters;
652 const struct xt_table_info *private = table->private;652 const struct xt_table_info *private = xt_table_get_private_protected(table);
653 653
654 /* We need atomic snapshot of counters: rest doesn't change654 /* We need atomic snapshot of counters: rest doesn't change
655 * (other than comefrom, which userspace doesn't care655 * (other than comefrom, which userspace doesn't care
@@ -673,7 +673,7 @@ static int copy_entries_to_user(unsigned int total_size, · 17 unmodified lines
673 unsigned int off, num;673 unsigned int off, num;
674 const struct arpt_entry *e;674 const struct arpt_entry *e;
675 struct xt_counters *counters;675 struct xt_counters *counters;
676 struct xt_table_info *private = table->private;676 struct xt_table_info *private = xt_table_get_private_protected(table);
677 int ret = 0;677 int ret = 0;
678 void *loc_cpu_entry;678 void *loc_cpu_entry;
679 679
@@ -807,7 +807,7 @@ static int get_info(struct net *net, void __user *user, const int *len) · 127 unmodified lines
807 t = xt_request_find_table_lock(net, NFPROTO_ARP, name);807 t = xt_request_find_table_lock(net, NFPROTO_ARP, name);
808 if (!IS_ERR(t)) {808 if (!IS_ERR(t)) {
809 struct arpt_getinfo info;809 struct arpt_getinfo info;
810 const struct xt_table_info *private = t->private;810 const struct xt_table_info *private = xt_table_get_private_protected(t);
811#ifdef CONFIG_COMPAT811#ifdef CONFIG_COMPAT
812 struct xt_table_info tmp;812 struct xt_table_info tmp;
813 813
@@ -860,7 +860,7 @@ static int get_entries(struct net *net, struct arpt_get_entries __user *uptr, · 46 unmodified lines
860 860
861 t = xt_find_table_lock(net, NFPROTO_ARP, get.name);861 t = xt_find_table_lock(net, NFPROTO_ARP, get.name);
862 if (!IS_ERR(t)) {862 if (!IS_ERR(t)) {
863 const struct xt_table_info *private = t->private;863 const struct xt_table_info *private = xt_table_get_private_protected(t);
864 864
865 if (get.size == private->size)865 if (get.size == private->size)
866 ret = copy_entries_to_user(private->size,866 ret = copy_entries_to_user(private->size,
@@ -1017,7 +1017,7 @@ static int do_add_counters(struct net *net, sockptr_t arg, unsigned int len) · 150 unmodified lines
1017 }1017 }
1018 1018
1019 local_bh_disable();1019 local_bh_disable();
1020 private = t->private;1020 private = xt_table_get_private_protected(t);
1021 if (private->number != tmp.num_counters) {1021 if (private->number != tmp.num_counters) {
1022 ret = -EINVAL;1022 ret = -EINVAL;
1023 goto unlock_up_free;1023 goto unlock_up_free;
@@ -1330,7 +1330,7 @@ static int compat_copy_entries_to_user(unsigned int total_size, · 306 unmodified lines
1330 void __user *userptr)1330 void __user *userptr)
1331{1331{
1332 struct xt_counters *counters;1332 struct xt_counters *counters;
1333 const struct xt_table_info *private = table->private;1333 const struct xt_table_info *private = xt_table_get_private_protected(table);
1334 void __user *pos;1334 void __user *pos;
1335 unsigned int size;1335 unsigned int size;
1336 int ret = 0;1336 int ret = 0;
net/ipv4/netfilter/ip_tables.c+7−7
@@ -258,7 +258,7 @@ ipt_do_table(struct sk_buff *skb, · 257 unmodified lines
258 WARN_ON(!(table->valid_hooks & (1 << hook)));258 WARN_ON(!(table->valid_hooks & (1 << hook)));
259 local_bh_disable();259 local_bh_disable();
260 addend = xt_write_recseq_begin();260 addend = xt_write_recseq_begin();
261 private = READ_ONCE(table->private); /* Address dependency. */261 private = rcu_access_pointer(table->private);
262 cpu = smp_processor_id();262 cpu = smp_processor_id();
263 table_base = private->entries;263 table_base = private->entries;
264 jumpstack = (struct ipt_entry **)private->jumpstack[cpu];264 jumpstack = (struct ipt_entry **)private->jumpstack[cpu];
@@ -791,7 +791,7 @@ static struct xt_counters *alloc_counters(const struct xt_table *table) · 526 unmodified lines
791{791{
792 unsigned int countersize;792 unsigned int countersize;
793 struct xt_counters *counters;793 struct xt_counters *counters;
794 const struct xt_table_info *private = table->private;794 const struct xt_table_info *private = xt_table_get_private_protected(table);
795 795
796 /* We need atomic snapshot of counters: rest doesn't change796 /* We need atomic snapshot of counters: rest doesn't change
797 (other than comefrom, which userspace doesn't care797 (other than comefrom, which userspace doesn't care
@@ -815,7 +815,7 @@ copy_entries_to_user(unsigned int total_size, · 17 unmodified lines
815 unsigned int off, num;815 unsigned int off, num;
816 const struct ipt_entry *e;816 const struct ipt_entry *e;
817 struct xt_counters *counters;817 struct xt_counters *counters;
818 const struct xt_table_info *private = table->private;818 const struct xt_table_info *private = xt_table_get_private_protected(table);
819 int ret = 0;819 int ret = 0;
820 const void *loc_cpu_entry;820 const void *loc_cpu_entry;
821 821
@@ -964,7 +964,7 @@ static int get_info(struct net *net, void __user *user, const int *len) · 142 unmodified lines
964 t = xt_request_find_table_lock(net, AF_INET, name);964 t = xt_request_find_table_lock(net, AF_INET, name);
965 if (!IS_ERR(t)) {965 if (!IS_ERR(t)) {
966 struct ipt_getinfo info;966 struct ipt_getinfo info;
967 const struct xt_table_info *private = t->private;967 const struct xt_table_info *private = xt_table_get_private_protected(t);
968#ifdef CONFIG_COMPAT968#ifdef CONFIG_COMPAT
969 struct xt_table_info tmp;969 struct xt_table_info tmp;
970 970
@@ -1018,7 +1018,7 @@ get_entries(struct net *net, struct ipt_get_entries __user *uptr, · 47 unmodified lines
1018 1018
1019 t = xt_find_table_lock(net, AF_INET, get.name);1019 t = xt_find_table_lock(net, AF_INET, get.name);
1020 if (!IS_ERR(t)) {1020 if (!IS_ERR(t)) {
1021 const struct xt_table_info *private = t->private;1021 const struct xt_table_info *private = xt_table_get_private_protected(t);
1022 if (get.size == private->size)1022 if (get.size == private->size)
1023 ret = copy_entries_to_user(private->size,1023 ret = copy_entries_to_user(private->size,
1024 t, uptr->entrytable);1024 t, uptr->entrytable);
@@ -1173,7 +1173,7 @@ do_add_counters(struct net *net, sockptr_t arg, unsigned int len) · 148 unmodified lines
1173 }1173 }
1174 1174
1175 local_bh_disable();1175 local_bh_disable();
1176 private = t->private;1176 private = xt_table_get_private_protected(t);
1177 if (private->number != tmp.num_counters) {1177 if (private->number != tmp.num_counters) {
1178 ret = -EINVAL;1178 ret = -EINVAL;
1179 goto unlock_up_free;1179 goto unlock_up_free;
@@ -1543,7 +1543,7 @@ compat_copy_entries_to_user(unsigned int total_size, struct xt_table *table, · 363 unmodified lines
1543 void __user *userptr)1543 void __user *userptr)
1544{1544{
1545 struct xt_counters *counters;1545 struct xt_counters *counters;
1546 const struct xt_table_info *private = table->private;1546 const struct xt_table_info *private = xt_table_get_private_protected(table);
1547 void __user *pos;1547 void __user *pos;
1548 unsigned int size;1548 unsigned int size;
1549 int ret = 0;1549 int ret = 0;
net/ipv6/netfilter/ip6_tables.c+7−7
@@ -280,7 +280,7 @@ ip6t_do_table(struct sk_buff *skb, · 279 unmodified lines
280 280
281 local_bh_disable();281 local_bh_disable();
282 addend = xt_write_recseq_begin();282 addend = xt_write_recseq_begin();
283 private = READ_ONCE(table->private); /* Address dependency. */283 private = rcu_access_pointer(table->private);
284 cpu = smp_processor_id();284 cpu = smp_processor_id();
285 table_base = private->entries;285 table_base = private->entries;
286 jumpstack = (struct ip6t_entry **)private->jumpstack[cpu];286 jumpstack = (struct ip6t_entry **)private->jumpstack[cpu];
@@ -807,7 +807,7 @@ static struct xt_counters *alloc_counters(const struct xt_table *table) · 520 unmodified lines
807{807{
808 unsigned int countersize;808 unsigned int countersize;
809 struct xt_counters *counters;809 struct xt_counters *counters;
810 const struct xt_table_info *private = table->private;810 const struct xt_table_info *private = xt_table_get_private_protected(table);
811 811
812 /* We need atomic snapshot of counters: rest doesn't change812 /* We need atomic snapshot of counters: rest doesn't change
813 (other than comefrom, which userspace doesn't care813 (other than comefrom, which userspace doesn't care
@@ -831,7 +831,7 @@ copy_entries_to_user(unsigned int total_size, · 17 unmodified lines
831 unsigned int off, num;831 unsigned int off, num;
832 const struct ip6t_entry *e;832 const struct ip6t_entry *e;
833 struct xt_counters *counters;833 struct xt_counters *counters;
834 const struct xt_table_info *private = table->private;834 const struct xt_table_info *private = xt_table_get_private_protected(table);
835 int ret = 0;835 int ret = 0;
836 const void *loc_cpu_entry;836 const void *loc_cpu_entry;
837 837
@@ -980,7 +980,7 @@ static int get_info(struct net *net, void __user *user, const int *len) · 142 unmodified lines
980 t = xt_request_find_table_lock(net, AF_INET6, name);980 t = xt_request_find_table_lock(net, AF_INET6, name);
981 if (!IS_ERR(t)) {981 if (!IS_ERR(t)) {
982 struct ip6t_getinfo info;982 struct ip6t_getinfo info;
983 const struct xt_table_info *private = t->private;983 const struct xt_table_info *private = xt_table_get_private_protected(t);
984#ifdef CONFIG_COMPAT984#ifdef CONFIG_COMPAT
985 struct xt_table_info tmp;985 struct xt_table_info tmp;
986 986
@@ -1035,7 +1035,7 @@ get_entries(struct net *net, struct ip6t_get_entries __user *uptr, · 48 unmodified lines
1035 1035
1036 t = xt_find_table_lock(net, AF_INET6, get.name);1036 t = xt_find_table_lock(net, AF_INET6, get.name);
1037 if (!IS_ERR(t)) {1037 if (!IS_ERR(t)) {
1038 struct xt_table_info *private = t->private;1038 struct xt_table_info *private = xt_table_get_private_protected(t);
1039 if (get.size == private->size)1039 if (get.size == private->size)
1040 ret = copy_entries_to_user(private->size,1040 ret = copy_entries_to_user(private->size,
1041 t, uptr->entrytable);1041 t, uptr->entrytable);
@@ -1189,7 +1189,7 @@ do_add_counters(struct net *net, sockptr_t arg, unsigned int len) · 147 unmodified lines
1189 }1189 }
1190 1190
1191 local_bh_disable();1191 local_bh_disable();
1192 private = t->private;1192 private = xt_table_get_private_protected(t);
1193 if (private->number != tmp.num_counters) {1193 if (private->number != tmp.num_counters) {
1194 ret = -EINVAL;1194 ret = -EINVAL;
1195 goto unlock_up_free;1195 goto unlock_up_free;
@@ -1552,7 +1552,7 @@ compat_copy_entries_to_user(unsigned int total_size, struct xt_table *table, · 356 unmodified lines
1552 void __user *userptr)1552 void __user *userptr)
1553{1553{
1554 struct xt_counters *counters;1554 struct xt_counters *counters;
1555 const struct xt_table_info *private = table->private;1555 const struct xt_table_info *private = xt_table_get_private_protected(table);
1556 void __user *pos;1556 void __user *pos;
1557 unsigned int size;1557 unsigned int size;
1558 int ret = 0;1558 int ret = 0;
net/netfilter/x_tables.c+15−34
@@ -1349,6 +1349,14 @@ struct xt_counters *xt_counters_alloc(unsigned int counters) · 1348 unmodified lines
1349}1349}
1350EXPORT_SYMBOL(xt_counters_alloc);1350EXPORT_SYMBOL(xt_counters_alloc);
1351 1351
1352struct xt_table_info
1353*xt_table_get_private_protected(const struct xt_table *table)
1354{
1355 return rcu_dereference_protected(table->private,
1356 mutex_is_locked(&xt[table->af].mutex));
1357}
1358EXPORT_SYMBOL(xt_table_get_private_protected);
1359
1352struct xt_table_info *1360struct xt_table_info *
1353xt_replace_table(struct xt_table *table,1361xt_replace_table(struct xt_table *table,
1354 unsigned int num_counters,1362 unsigned int num_counters,
@@ -1356,7 +1364,6 @@ xt_replace_table(struct xt_table *table, · 1 unmodified line
1356 int *error)1364 int *error)
1357{1365{
1358 struct xt_table_info *private;1366 struct xt_table_info *private;
1359 unsigned int cpu;
1360 int ret;1367 int ret;
1361 1368
1362 ret = xt_jumpstack_alloc(newinfo);1369 ret = xt_jumpstack_alloc(newinfo);
@@ -1366,47 +1373,20 @@ xt_replace_table(struct xt_table *table, · 3 unmodified lines
1366 }1373 }
1367 1374
1368 /* Do the substitution. */1375 /* Do the substitution. */
1369 local_bh_disable();1376 private = xt_table_get_private_protected(table);
1370 private = table->private;
1371 1377
1372 /* Check inside lock: is the old number correct? */1378 /* Check inside lock: is the old number correct? */
1373 if (num_counters != private->number) {1379 if (num_counters != private->number) {
1374 pr_debug("num_counters != table->private->number (%u/%u)\n",1380 pr_debug("num_counters != table->private->number (%u/%u)\n",
1375 num_counters, private->number);1381 num_counters, private->number);
1376 local_bh_enable();
1377 *error = -EAGAIN;1382 *error = -EAGAIN;
1378 return NULL;1383 return NULL;
1379 }1384 }
1380 1385
1381 newinfo->initial_entries = private->initial_entries;1386 newinfo->initial_entries = private->initial_entries;
1382 /*
1383 * Ensure contents of newinfo are visible before assigning to
1384 * private.
1385 */
1386 smp_wmb();
1387 table->private = newinfo;
1388
1389 /* make sure all cpus see new ->private value */
1390 smp_wmb();
1391 1387
1392 /*1388 rcu_assign_pointer(table->private, newinfo);
1393 * Even though table entries have now been swapped, other CPU's1389 synchronize_rcu();
1394 * may still be using the old entries...
1395 */
1396 local_bh_enable();
1397
1398 /* ... so wait for even xt_recseq on all cpus */
1399 for_each_possible_cpu(cpu) {
1400 seqcount_t *s = &per_cpu(xt_recseq, cpu);
1401 u32 seq = raw_read_seqcount(s);
1402
1403 if (seq & 1) {
1404 do {
1405 cond_resched();
1406 cpu_relax();
1407 } while (seq == raw_read_seqcount(s));
1408 }
1409 }
1410 1390
1411 audit_log_nfcfg(table->name, table->af, private->number,1391 audit_log_nfcfg(table->name, table->af, private->number,
1412 !private->number ? AUDIT_XT_OP_REGISTER :1392 !private->number ? AUDIT_XT_OP_REGISTER :
@@ -1442,12 +1422,12 @@ struct xt_table *xt_register_table(struct net *net, · 29 unmodified lines
1442 }1422 }
1443 1423
1444 /* Simplifies replace_table code. */1424 /* Simplifies replace_table code. */
1445 table->private = bootstrap;1425 rcu_assign_pointer(table->private, bootstrap);
1446 1426
1447 if (!xt_replace_table(table, 0, newinfo, &ret))1427 if (!xt_replace_table(table, 0, newinfo, &ret))
1448 goto unlock;1428 goto unlock;
1449 1429
1450 private = table->private;1430 private = xt_table_get_private_protected(table);
1451 pr_debug("table->private->number = %u\n", private->number);1431 pr_debug("table->private->number = %u\n", private->number);
1452 1432
1453 /* save number of initial entries */1433 /* save number of initial entries */
@@ -1470,7 +1450,8 @@ void *xt_unregister_table(struct xt_table *table) · 16 unmodified lines
1470 struct xt_table_info *private;1450 struct xt_table_info *private;
1471 1451
1472 mutex_lock(&xt[table->af].mutex);1452 mutex_lock(&xt[table->af].mutex);
1473 private = table->private;1453 private = xt_table_get_private_protected(table);
1454 RCU_INIT_POINTER(table->private, NULL);
1474 list_del(&table->list);1455 list_del(&table->list);
1475 mutex_unlock(&xt[table->af].mutex);1456 mutex_unlock(&xt[table->af].mutex);
1476 audit_log_nfcfg(table->name, table->af, private->number,1457 audit_log_nfcfg(table->name, table->af, private->number,