Commit e58a171d authored by Florian Westphal's avatar Florian Westphal Committed by Pablo Neira Ayuso

netfilter: ebtables: fix table blob use-after-free

We are not allowed to return an error at this point.
Looking at the code it looks like ret is always 0 at this
point, but its not.

t = find_table_lock(net, repl->name, &ret, &ebt_mutex);

... this can return a valid table, with ret != 0.

This bug causes update of table->private with the new
blob, but then frees the blob right away in the caller.

Syzbot report:

BUG: KASAN: vmalloc-out-of-bounds in __ebt_unregister_table+0xc00/0xcd0 net/bridge/netfilter/ebtables.c:1168
Read of size 4 at addr ffffc90005425000 by task kworker/u4:4/74
Workqueue: netns cleanup_net
Call Trace:
 kasan_report+0xbf/0x1f0 mm/kasan/report.c:517
 __ebt_unregister_table+0xc00/0xcd0 net/bridge/netfilter/ebtables.c:1168
 ebt_unregister_table+0x35/0x40 net/bridge/netfilter/ebtables.c:1372
 ops_exit_list+0xb0/0x170 net/core/net_namespace.c:169
 cleanup_net+0x4ee/0xb10 net/core/net_namespace.c:613
...

ip(6)tables appears to be ok (ret should be 0 at this point) but make
this more obvious.

Fixes: c58dd2dd ("netfilter: Can't fail and free after table replacement")
Reported-by: syzbot+f61594de72d6705aea03@syzkaller.appspotmail.com
Signed-off-by: default avatarFlorian Westphal <fw@strlen.de>
Signed-off-by: default avatarPablo Neira Ayuso <pablo@netfilter.org>
parent efb056e5
...@@ -1090,7 +1090,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, ...@@ -1090,7 +1090,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
audit_log_nfcfg(repl->name, AF_BRIDGE, repl->nentries, audit_log_nfcfg(repl->name, AF_BRIDGE, repl->nentries,
AUDIT_XT_OP_REPLACE, GFP_KERNEL); AUDIT_XT_OP_REPLACE, GFP_KERNEL);
return ret; return 0;
free_unlock: free_unlock:
mutex_unlock(&ebt_mutex); mutex_unlock(&ebt_mutex);
......
...@@ -1045,7 +1045,6 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, ...@@ -1045,7 +1045,6 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
struct xt_counters *counters; struct xt_counters *counters;
struct ipt_entry *iter; struct ipt_entry *iter;
ret = 0;
counters = xt_counters_alloc(num_counters); counters = xt_counters_alloc(num_counters);
if (!counters) { if (!counters) {
ret = -ENOMEM; ret = -ENOMEM;
...@@ -1091,7 +1090,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, ...@@ -1091,7 +1090,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
net_warn_ratelimited("iptables: counters copy to user failed while replacing table\n"); net_warn_ratelimited("iptables: counters copy to user failed while replacing table\n");
} }
vfree(counters); vfree(counters);
return ret; return 0;
put_module: put_module:
module_put(t->me); module_put(t->me);
......
...@@ -1062,7 +1062,6 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, ...@@ -1062,7 +1062,6 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
struct xt_counters *counters; struct xt_counters *counters;
struct ip6t_entry *iter; struct ip6t_entry *iter;
ret = 0;
counters = xt_counters_alloc(num_counters); counters = xt_counters_alloc(num_counters);
if (!counters) { if (!counters) {
ret = -ENOMEM; ret = -ENOMEM;
...@@ -1108,7 +1107,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, ...@@ -1108,7 +1107,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
net_warn_ratelimited("ip6tables: counters copy to user failed while replacing table\n"); net_warn_ratelimited("ip6tables: counters copy to user failed while replacing table\n");
} }
vfree(counters); vfree(counters);
return ret; return 0;
put_module: put_module:
module_put(t->me); module_put(t->me);
......
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