Skip to contentSkip to conversation

Patch "netfilter: x_tables: do compat validation via translate_table" has been added to the 4.4-stable tree

PATCH [email protected] 1 participant 1 message Jun 23, 2016

Subsystems (3)
lore
Share & tools
lore

[email protected]

author maintainer Applied
#0 lore raw

This is a note to let you know that I've just added the patch titled

netfilter: x_tables: do compat validation via translate_table

to the 4.4-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
netfilter-x_tables-do-compat-validation-via-translate_table.patch
and it can be found in the queue-4.4 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From 09d9686047dbbe1cf4faa558d3ecc4aae2046054 Mon Sep 17 00:00:00 2001

From: Florian Westphal <[email protected]>
Date: Fri, 1 Apr 2016 14:17:34 +0200
Subject: netfilter: x_tables: do compat validation via translate_table

From: Florian Westphal <[email protected]>

commit 09d9686047dbbe1cf4faa558d3ecc4aae2046054 upstream.

This looks like refactoring, but its also a bug fix.

Problem is that the compat path (32bit iptables, 64bit kernel) lacks a few sanity tests that are done in the normal path.

For example, we do not check for underflows and the base chain policies.

While its possible to also add such checks to the compat path, its more copy&pastry, for instance we cannot reuse check_underflow() helper as e->target_offset differs in the compat case.

Other problem is that it makes auditing for validation errors harder; two places need to be checked and kept in sync.

At a high level 32 bit compat works like this:
1- initial pass over blob:
validate match/entry offsets, bounds checking lookup all matches and targets
do bookkeeping wrt. size delta of 32/64bit structures assign match/target.u.kernel pointer (points at kernel implementation, needed to access ->compatsize etc.)

2- allocate memory according to the total bookkeeping size to
contain the translated ruleset

3- second pass over original blob:
for each entry, copy the 32bit representation to the newly allocated memory. This also does any special match translations (e.g. adjust 32bit to 64bit longs, etc).

4- check if ruleset is free of loops (chase all jumps)

5-first pass over translated blob:
call the checkentry function of all matches and targets.

The alternative implemented by this patch is to drop steps 3&4 from the compat process, the translation is changed into an intermediate step rather than a full 1:1 translate_table replacement.

In the 2nd pass (step #3), change the 64bit ruleset back to a kernel representation, i.e. put() the kernel pointer and restore ->u.user.name .

This gets us a 64bit ruleset that is in the format generated by a 64bit iptables userspace -- we can then use translate_table() to get the 'native' sanity checks.

This has two drawbacks:

1. we re-validate all the match and target entry structure sizes even though compat translation is supposed to never generate bogus offsets.
2. we put and then re-lookup each match and target.

THe upside is that we get all sanity tests and ruleset validations provided by the normal path and can remove some duplicated compat code.

iptables-restore time of autogenerated ruleset with 300k chains of form -A CHAIN0001 -m limit --limit 1/s -j CHAIN0002
-A CHAIN0002 -m limit --limit 1/s -j CHAIN0003

shows no noticeable differences in restore times:

old:   0m30.796s
new:   0m31.521s

64bit: 0m25.674s

---
 net/ipv4/netfilter/arp_tables.c |  114 +++++------------------------
 net/ipv4/netfilter/ip_tables.c  |  155 +++++++---------------------------------
 net/ipv6/netfilter/ip6_tables.c |  148 +++++---------------------------------
 net/netfilter/x_tables.c        |    8 ++
 4 files changed, 83 insertions(+), 342 deletions(-)
net/ipv4/netfilter/arp_tables.c +23 -91
@@ -1233,19 +1233,17 @@ static inline void compat_release_entry(
 	module_put(t->u.kernel.target->me);
 }
 
-static inline int
+static int
 check_compat_entry_size_and_hooks(struct compat_arpt_entry *e,
 				  struct xt_table_info *newinfo,
 				  unsigned int *size,
 				  const unsigned char *base,
-				  const unsigned char *limit,
-				  const unsigned int *hook_entries,
-				  const unsigned int *underflows)
+				  const unsigned char *limit)
 {
 	struct xt_entry_target *t;
 	struct xt_target *target;
 	unsigned int entry_offset;
-	int ret, off, h;
+	int ret, off;
 
 	duprintf("check_compat_entry_size_and_hooks %p\n", e);
 	if ((unsigned long)e % __alignof__(struct compat_arpt_entry) != 0 ||
@@ -1290,17 +1288,6 @@ check_compat_entry_size_and_hooks(struct
 	if (ret)
 		goto release_target;
 
-	/* Check hooks & underflows */
-	for (h = 0; h < NF_ARP_NUMHOOKS; h++) {
-		if ((unsigned char *)e - base == hook_entries[h])
-			newinfo->hook_entry[h] = hook_entries[h];
-		if ((unsigned char *)e - base == underflows[h])
-			newinfo->underflow[h] = underflows[h];
-	}
-
-	/* Clear counters and comefrom */
-	memset(&e->counters, 0, sizeof(e->counters));
-	e->comefrom = 0;
 	return 0;
 
 release_target:
@@ -1350,7 +1337,7 @@ static int translate_compat_table(struct
 	struct xt_table_info *newinfo, *info;
 	void *pos, *entry0, *entry1;
 	struct compat_arpt_entry *iter0;
-	struct arpt_entry *iter1;
+	struct arpt_replace repl;
 	unsigned int size;
 	int ret = 0;
 
@@ -1359,12 +1346,6 @@ static int translate_compat_table(struct
 	size = compatr->size;
 	info->number = compatr->num_entries;
 
-	/* Init all hooks to impossible value. */
-	for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
-		info->hook_entry[i] = 0xFFFFFFFF;
-		info->underflow[i] = 0xFFFFFFFF;
-	}
-
 	duprintf("translate_compat_table: size %u\n", info->size);
 	j = 0;
 	xt_compat_lock(NFPROTO_ARP);
@@ -1373,9 +1354,7 @@ static int translate_compat_table(struct
 	xt_entry_foreach(iter0, entry0, compatr->size) {
 		ret = check_compat_entry_size_and_hooks(iter0, info, &size,
 							entry0,
-							entry0 + compatr->size,
-							compatr->hook_entry,
-							compatr->underflow);
+							entry0 + compatr->size);
 		if (ret != 0)
 			goto out_unlock;
 		++j;
@@ -1388,23 +1367,6 @@ static int translate_compat_table(struct
 		goto out_unlock;
 	}
 
-	/* Check hooks all assigned */
-	for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
-		/* Only hooks which are valid */
-		if (!(compatr->valid_hooks & (1 << i)))
-			continue;
-		if (info->hook_entry[i] == 0xFFFFFFFF) {
-			duprintf("Invalid hook entry %u %u\n",
-				 i, info->hook_entry[i]);
-			goto out_unlock;
-		}
-		if (info->underflow[i] == 0xFFFFFFFF) {
-			duprintf("Invalid underflow %u %u\n",
-				 i, info->underflow[i]);
-			goto out_unlock;
-		}
-	}
-
 	ret = -ENOMEM;
 	newinfo = xt_alloc_table_info(size);
 	if (!newinfo)
@@ -1421,55 +1383,26 @@ static int translate_compat_table(struct
 	xt_entry_foreach(iter0, entry0, compatr->size)
 		compat_copy_entry_from_user(iter0, &pos, &size,
 					    newinfo, entry1);
+
+	/* all module references in entry0 are now gone */
+
 	xt_compat_flush_offsets(NFPROTO_ARP);
 	xt_compat_unlock(NFPROTO_ARP);
 
-	ret = -ELOOP;
-	if (!mark_source_chains(newinfo, compatr->valid_hooks, entry1))
-		goto free_newinfo;
-
-	i = 0;
-	xt_entry_foreach(iter1, entry1, newinfo->size) {
-		iter1->counters.pcnt = xt_percpu_counter_alloc();
-		if (IS_ERR_VALUE(iter1->counters.pcnt)) {
-			ret = -ENOMEM;
-			break;
-		}
+	memcpy(&repl, compatr, sizeof(*compatr));
 
-		ret = check_target(iter1, compatr->name);
-		if (ret != 0) {
-			xt_percpu_counter_free(iter1->counters.pcnt);
-			break;
-		}
-		++i;
-		if (strcmp(arpt_get_target(iter1)->u.user.name,
-		    XT_ERROR_TARGET) == 0)
-			++newinfo->stacksize;
-	}
-	if (ret) {
-		/*
-		 * The first i matches need cleanup_entry (calls ->destroy)
-		 * because they had called ->check already. The other j-i
-		 * entries need only release.
-		 */
-		int skip = i;
-		j -= i;
-		xt_entry_foreach(iter0, entry0, newinfo->size) {
-			if (skip-- > 0)
-				continue;
-			if (j-- == 0)
-				break;
-			compat_release_entry(iter0);
-		}
-		xt_entry_foreach(iter1, entry1, newinfo->size) {
-			if (i-- == 0)
-				break;
-			cleanup_entry(iter1);
-		}
-		xt_free_table_info(newinfo);
-		return ret;
+	for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
+		repl.hook_entry[i] = newinfo->hook_entry[i];
+		repl.underflow[i] = newinfo->underflow[i];
 	}
 
+	repl.num_counters = 0;
+	repl.counters = NULL;
+	repl.size = newinfo->size;
+	ret = translate_table(newinfo, entry1, &repl);
+	if (ret)
+		goto free_newinfo;
+
 	*pinfo = newinfo;
 	*pentry0 = entry1;
 	xt_free_table_info(info);
@@ -1477,17 +1410,16 @@ static int translate_compat_table(struct
 
 free_newinfo:
 	xt_free_table_info(newinfo);
-out:
+	return ret;
+out_unlock:
+	xt_compat_flush_offsets(NFPROTO_ARP);
+	xt_compat_unlock(NFPROTO_ARP);
 	xt_entry_foreach(iter0, entry0, compatr->size) {
 		if (j-- == 0)
 			break;
 		compat_release_entry(iter0);
 	}
 	return ret;
-out_unlock:
-	xt_compat_flush_offsets(NFPROTO_ARP);
-	xt_compat_unlock(NFPROTO_ARP);
-	goto out;
 }
 
 static int compat_do_replace(struct net *net, void __user *user,
net/ipv4/netfilter/ip_tables.c +29 -126

239 lines not on this page · raw message

net/ipv6/netfilter/ip6_tables.c +23 -125

227 lines not on this page · raw message

net/netfilter/x_tables.c +8 -0
@@ -532,6 +532,7 @@ void xt_compat_match_from_user(struct xt
 	struct compat_xt_entry_match *cm = (struct compat_xt_entry_match *)m;
 	int pad, off = xt_compat_match_offset(match);
 	u_int16_t msize = cm->u.user.match_size;
+	char name[sizeof(m->u.user.name)];
 
 	m = *dstptr;
 	memcpy(m, cm, sizeof(*cm));
@@ -545,6 +546,9 @@ void xt_compat_match_from_user(struct xt
 
 	msize += off;
 	m->u.user.match_size = msize;
+	strlcpy(name, match->name, sizeof(name));
+	module_put(match->me);
+	strncpy(m->u.user.name, name, sizeof(m->u.user.name));
 
 	*size += off;
 	*dstptr += msize;
@@ -762,6 +766,7 @@ void xt_compat_target_from_user(struct x
 	struct compat_xt_entry_target *ct = (struct compat_xt_entry_target *)t;
 	int pad, off = xt_compat_target_offset(target);
 	u_int16_t tsize = ct->u.user.target_size;
+	char name[sizeof(t->u.user.name)];
 
 	t = *dstptr;
 	memcpy(t, ct, sizeof(*ct));
@@ -775,6 +780,9 @@ void xt_compat_target_from_user(struct x
 
 	tsize += off;
 	t->u.user.target_size = tsize;
+	strlcpy(name, target->name, sizeof(name));
+	module_put(target->me);
+	strncpy(t->u.user.name, name, sizeof(t->u.user.name));
 
 	*size += off;
 	*dstptr += tsize;

Patches currently in stable-queue which might be from [email protected] are

queue-4.4/netfilter-x_tables-validate-targets-of-jumps.patch queue-4.4/netfilter-x_tables-introduce-and-use-xt_copy_counters_from_user.patch queue-4.4/netfilter-arp_tables-simplify-translate_compat_table-args.patch queue-4.4/netfilter-x_tables-validate-e-target_offset-early.patch queue-4.4/netfilter-x_tables-assert-minimum-target-size.patch queue-4.4/netfilter-ip6_tables-simplify-translate_compat_table-args.patch queue-4.4/netfilter-x_tables-check-for-bogus-target-offset.patch queue-4.4/netfilter-x_tables-add-compat-version-of-xt_check_entry_offsets.patch queue-4.4/netfilter-x_tables-don-t-reject-valid-target-size-on-some-architectures.patch queue-4.4/netfilter-x_tables-check-standard-target-size-too.patch queue-4.4/netfilter-x_tables-xt_compat_match_from_user-doesn-t-need-a-retval.patch queue-4.4/netfilter-x_tables-do-compat-validation-via-translate_table.patch queue-4.4/netfilter-x_tables-add-and-use-xt_check_entry_offsets.patch queue-4.4/netfilter-x_tables-don-t-move-to-non-existent-next-rule.patch queue-4.4/netfilter-x_tables-kill-check_entry-helper.patch queue-4.4/netfilter-x_tables-validate-all-offsets-and-sizes-in-a-rule.patch queue-4.4/netfilter-x_tables-make-sure-e-next_offset-covers-remaining-blob-size.patch queue-4.4/netfilter-ip_tables-simplify-translate_compat_table-args.patch queue-4.4/netfilter-x_tables-fix-unconditional-helper.patch