Skip to content

Commit 09d9686

Browse files
Florian Westphalummakynes
Florian Westphal
authored and
ummakynes
committed
netfilter: x_tables: do compat validation via translate_table
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 Signed-off-by: Florian Westphal <fw@strlen.de> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
1 parent 0188346 commit 09d9686

File tree

4 files changed

+83
-342
lines changed

4 files changed

+83
-342
lines changed

‎net/ipv4/netfilter/arp_tables.c

Lines changed: 23 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -1234,19 +1234,17 @@ static inline void compat_release_entry(struct compat_arpt_entry *e)
12341234
module_put(t->u.kernel.target->me);
12351235
}
12361236

1237-
static inline int
1237+
static int
12381238
check_compat_entry_size_and_hooks(struct compat_arpt_entry *e,
12391239
struct xt_table_info *newinfo,
12401240
unsigned int *size,
12411241
const unsigned char *base,
1242-
const unsigned char *limit,
1243-
const unsigned int *hook_entries,
1244-
const unsigned int *underflows)
1242+
const unsigned char *limit)
12451243
{
12461244
struct xt_entry_target *t;
12471245
struct xt_target *target;
12481246
unsigned int entry_offset;
1249-
int ret, off, h;
1247+
int ret, off;
12501248

12511249
duprintf("check_compat_entry_size_and_hooks %p\n", e);
12521250
if ((unsigned long)e % __alignof__(struct compat_arpt_entry) != 0 ||
@@ -1291,17 +1289,6 @@ check_compat_entry_size_and_hooks(struct compat_arpt_entry *e,
12911289
if (ret)
12921290
goto release_target;
12931291

1294-
/* Check hooks & underflows */
1295-
for (h = 0; h < NF_ARP_NUMHOOKS; h++) {
1296-
if ((unsigned char *)e - base == hook_entries[h])
1297-
newinfo->hook_entry[h] = hook_entries[h];
1298-
if ((unsigned char *)e - base == underflows[h])
1299-
newinfo->underflow[h] = underflows[h];
1300-
}
1301-
1302-
/* Clear counters and comefrom */
1303-
memset(&e->counters, 0, sizeof(e->counters));
1304-
e->comefrom = 0;
13051292
return 0;
13061293

13071294
release_target:
@@ -1351,7 +1338,7 @@ static int translate_compat_table(struct xt_table_info **pinfo,
13511338
struct xt_table_info *newinfo, *info;
13521339
void *pos, *entry0, *entry1;
13531340
struct compat_arpt_entry *iter0;
1354-
struct arpt_entry *iter1;
1341+
struct arpt_replace repl;
13551342
unsigned int size;
13561343
int ret = 0;
13571344

@@ -1360,12 +1347,6 @@ static int translate_compat_table(struct xt_table_info **pinfo,
13601347
size = compatr->size;
13611348
info->number = compatr->num_entries;
13621349

1363-
/* Init all hooks to impossible value. */
1364-
for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
1365-
info->hook_entry[i] = 0xFFFFFFFF;
1366-
info->underflow[i] = 0xFFFFFFFF;
1367-
}
1368-
13691350
duprintf("translate_compat_table: size %u\n", info->size);
13701351
j = 0;
13711352
xt_compat_lock(NFPROTO_ARP);
@@ -1374,9 +1355,7 @@ static int translate_compat_table(struct xt_table_info **pinfo,
13741355
xt_entry_foreach(iter0, entry0, compatr->size) {
13751356
ret = check_compat_entry_size_and_hooks(iter0, info, &size,
13761357
entry0,
1377-
entry0 + compatr->size,
1378-
compatr->hook_entry,
1379-
compatr->underflow);
1358+
entry0 + compatr->size);
13801359
if (ret != 0)
13811360
goto out_unlock;
13821361
++j;
@@ -1389,23 +1368,6 @@ static int translate_compat_table(struct xt_table_info **pinfo,
13891368
goto out_unlock;
13901369
}
13911370

1392-
/* Check hooks all assigned */
1393-
for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
1394-
/* Only hooks which are valid */
1395-
if (!(compatr->valid_hooks & (1 << i)))
1396-
continue;
1397-
if (info->hook_entry[i] == 0xFFFFFFFF) {
1398-
duprintf("Invalid hook entry %u %u\n",
1399-
i, info->hook_entry[i]);
1400-
goto out_unlock;
1401-
}
1402-
if (info->underflow[i] == 0xFFFFFFFF) {
1403-
duprintf("Invalid underflow %u %u\n",
1404-
i, info->underflow[i]);
1405-
goto out_unlock;
1406-
}
1407-
}
1408-
14091371
ret = -ENOMEM;
14101372
newinfo = xt_alloc_table_info(size);
14111373
if (!newinfo)
@@ -1422,73 +1384,43 @@ static int translate_compat_table(struct xt_table_info **pinfo,
14221384
xt_entry_foreach(iter0, entry0, compatr->size)
14231385
compat_copy_entry_from_user(iter0, &pos, &size,
14241386
newinfo, entry1);
1387+
1388+
/* all module references in entry0 are now gone */
1389+
14251390
xt_compat_flush_offsets(NFPROTO_ARP);
14261391
xt_compat_unlock(NFPROTO_ARP);
14271392

1428-
ret = -ELOOP;
1429-
if (!mark_source_chains(newinfo, compatr->valid_hooks, entry1))
1430-
goto free_newinfo;
1431-
1432-
i = 0;
1433-
xt_entry_foreach(iter1, entry1, newinfo->size) {
1434-
iter1->counters.pcnt = xt_percpu_counter_alloc();
1435-
if (IS_ERR_VALUE(iter1->counters.pcnt)) {
1436-
ret = -ENOMEM;
1437-
break;
1438-
}
1393+
memcpy(&repl, compatr, sizeof(*compatr));
14391394

1440-
ret = check_target(iter1, compatr->name);
1441-
if (ret != 0) {
1442-
xt_percpu_counter_free(iter1->counters.pcnt);
1443-
break;
1444-
}
1445-
++i;
1446-
if (strcmp(arpt_get_target(iter1)->u.user.name,
1447-
XT_ERROR_TARGET) == 0)
1448-
++newinfo->stacksize;
1449-
}
1450-
if (ret) {
1451-
/*
1452-
* The first i matches need cleanup_entry (calls ->destroy)
1453-
* because they had called ->check already. The other j-i
1454-
* entries need only release.
1455-
*/
1456-
int skip = i;
1457-
j -= i;
1458-
xt_entry_foreach(iter0, entry0, newinfo->size) {
1459-
if (skip-- > 0)
1460-
continue;
1461-
if (j-- == 0)
1462-
break;
1463-
compat_release_entry(iter0);
1464-
}
1465-
xt_entry_foreach(iter1, entry1, newinfo->size) {
1466-
if (i-- == 0)
1467-
break;
1468-
cleanup_entry(iter1);
1469-
}
1470-
xt_free_table_info(newinfo);
1471-
return ret;
1395+
for (i = 0; i < NF_ARP_NUMHOOKS; i++) {
1396+
repl.hook_entry[i] = newinfo->hook_entry[i];
1397+
repl.underflow[i] = newinfo->underflow[i];
14721398
}
14731399

1400+
repl.num_counters = 0;
1401+
repl.counters = NULL;
1402+
repl.size = newinfo->size;
1403+
ret = translate_table(newinfo, entry1, &repl);
1404+
if (ret)
1405+
goto free_newinfo;
1406+
14741407
*pinfo = newinfo;
14751408
*pentry0 = entry1;
14761409
xt_free_table_info(info);
14771410
return 0;
14781411

14791412
free_newinfo:
14801413
xt_free_table_info(newinfo);
1481-
out:
1414+
return ret;
1415+
out_unlock:
1416+
xt_compat_flush_offsets(NFPROTO_ARP);
1417+
xt_compat_unlock(NFPROTO_ARP);
14821418
xt_entry_foreach(iter0, entry0, compatr->size) {
14831419
if (j-- == 0)
14841420
break;
14851421
compat_release_entry(iter0);
14861422
}
14871423
return ret;
1488-
out_unlock:
1489-
xt_compat_flush_offsets(NFPROTO_ARP);
1490-
xt_compat_unlock(NFPROTO_ARP);
1491-
goto out;
14921424
}
14931425

14941426
static int compat_do_replace(struct net *net, void __user *user,

0 commit comments

Comments
 (0)