Commit 7997eff8 authored by Florian Westphal's avatar Florian Westphal

netfilter: ebtables: reject blobs that don't provide all entry points

Harshit Mogalapalli says:
 In ebt_do_table() function dereferencing 'private->hook_entry[hook]'
 can lead to NULL pointer dereference. [..] Kernel panic:

general protection fault, probably for non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN
KASAN: null-ptr-deref in range [0x0000000000000028-0x000000000000002f]
[..]
RIP: 0010:ebt_do_table+0x1dc/0x1ce0
Code: 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 5c 16 00 00 48 b8 00 00 00 00 00 fc ff df 49 8b 6c df 08 48 8d 7d 2c 48 89 fa 48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2 0f 85 88
[..]
Call Trace:
 nf_hook_slow+0xb1/0x170
 __br_forward+0x289/0x730
 maybe_deliver+0x24b/0x380
 br_flood+0xc6/0x390
 br_dev_xmit+0xa2e/0x12c0

For some reason ebtables rejects blobs that provide entry points that are
not supported by the table, but what it should instead reject is the
opposite: blobs that DO NOT provide an entry point supported by the table.

t->valid_hooks is the bitmask of hooks (input, forward ...) that will see
packets.  Providing an entry point that is not support is harmless
(never called/used), but the inverse isn't: it results in a crash
because the ebtables traverser doesn't expect a NULL blob for a location
its receiving packets for.

Instead of fixing all the individual checks, do what iptables is doing and
reject all blobs that differ from the expected hooks.

Fixes: 1da177e4 ("Linux-2.6.12-rc2")
Reported-by: default avatarHarshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Reported-by: default avatarsyzkaller <syzkaller@googlegroups.com>
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
parent 855a28f9
...@@ -94,10 +94,6 @@ struct ebt_table { ...@@ -94,10 +94,6 @@ struct ebt_table {
struct ebt_replace_kernel *table; struct ebt_replace_kernel *table;
unsigned int valid_hooks; unsigned int valid_hooks;
rwlock_t lock; rwlock_t lock;
/* e.g. could be the table explicitly only allows certain
* matches, targets, ... 0 == let it in */
int (*check)(const struct ebt_table_info *info,
unsigned int valid_hooks);
/* the data used by the kernel */ /* the data used by the kernel */
struct ebt_table_info *private; struct ebt_table_info *private;
struct nf_hook_ops *ops; struct nf_hook_ops *ops;
......
...@@ -36,18 +36,10 @@ static struct ebt_replace_kernel initial_table = { ...@@ -36,18 +36,10 @@ static struct ebt_replace_kernel initial_table = {
.entries = (char *)&initial_chain, .entries = (char *)&initial_chain,
}; };
static int check(const struct ebt_table_info *info, unsigned int valid_hooks)
{
if (valid_hooks & ~(1 << NF_BR_BROUTING))
return -EINVAL;
return 0;
}
static const struct ebt_table broute_table = { static const struct ebt_table broute_table = {
.name = "broute", .name = "broute",
.table = &initial_table, .table = &initial_table,
.valid_hooks = 1 << NF_BR_BROUTING, .valid_hooks = 1 << NF_BR_BROUTING,
.check = check,
.me = THIS_MODULE, .me = THIS_MODULE,
}; };
......
...@@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { ...@@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = {
.entries = (char *)initial_chains, .entries = (char *)initial_chains,
}; };
static int check(const struct ebt_table_info *info, unsigned int valid_hooks)
{
if (valid_hooks & ~FILTER_VALID_HOOKS)
return -EINVAL;
return 0;
}
static const struct ebt_table frame_filter = { static const struct ebt_table frame_filter = {
.name = "filter", .name = "filter",
.table = &initial_table, .table = &initial_table,
.valid_hooks = FILTER_VALID_HOOKS, .valid_hooks = FILTER_VALID_HOOKS,
.check = check,
.me = THIS_MODULE, .me = THIS_MODULE,
}; };
......
...@@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { ...@@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = {
.entries = (char *)initial_chains, .entries = (char *)initial_chains,
}; };
static int check(const struct ebt_table_info *info, unsigned int valid_hooks)
{
if (valid_hooks & ~NAT_VALID_HOOKS)
return -EINVAL;
return 0;
}
static const struct ebt_table frame_nat = { static const struct ebt_table frame_nat = {
.name = "nat", .name = "nat",
.table = &initial_table, .table = &initial_table,
.valid_hooks = NAT_VALID_HOOKS, .valid_hooks = NAT_VALID_HOOKS,
.check = check,
.me = THIS_MODULE, .me = THIS_MODULE,
}; };
......
...@@ -1040,8 +1040,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, ...@@ -1040,8 +1040,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
goto free_iterate; goto free_iterate;
} }
/* the table doesn't like it */ if (repl->valid_hooks != t->valid_hooks)
if (t->check && (ret = t->check(newinfo, repl->valid_hooks)))
goto free_unlock; goto free_unlock;
if (repl->num_counters && repl->num_counters != t->private->nentries) { if (repl->num_counters && repl->num_counters != t->private->nentries) {
...@@ -1231,11 +1230,6 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table, ...@@ -1231,11 +1230,6 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table,
if (ret != 0) if (ret != 0)
goto free_chainstack; goto free_chainstack;
if (table->check && table->check(newinfo, table->valid_hooks)) {
ret = -EINVAL;
goto free_chainstack;
}
table->private = newinfo; table->private = newinfo;
rwlock_init(&table->lock); rwlock_init(&table->lock);
mutex_lock(&ebt_mutex); mutex_lock(&ebt_mutex);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment