Skip to content

Commit c58dd2d

Browse files
tgrafummakynes
authored andcommitted
netfilter: Can't fail and free after table replacement
All xtables variants suffer from the defect that the copy_to_user() to copy the counters to user memory may fail after the table has already been exchanged and thus exposed. Return an error at this point will result in freeing the already exposed table. Any subsequent packet processing will result in a kernel panic. We can't copy the counters before exposing the new tables as we want provide the counter state after the old table has been unhooked. Therefore convert this into a silent error. Cc: Florian Westphal <fw@strlen.de> Signed-off-by: Thomas Graf <tgraf@suug.ch> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
1 parent 2fec6bb commit c58dd2d

File tree

4 files changed

+14
-9
lines changed

4 files changed

+14
-9
lines changed

net/bridge/netfilter/ebtables.c

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,10 +1044,9 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
10441044
if (repl->num_counters &&
10451045
copy_to_user(repl->counters, counterstmp,
10461046
repl->num_counters * sizeof(struct ebt_counter))) {
1047-
ret = -EFAULT;
1047+
/* Silent error, can't fail, new table is already in place */
1048+
net_warn_ratelimited("ebtables: counters copy to user failed while replacing table\n");
10481049
}
1049-
else
1050-
ret = 0;
10511050

10521051
/* decrease module count and free resources */
10531052
EBT_ENTRY_ITERATE(table->entries, table->entries_size,

net/ipv4/netfilter/arp_tables.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1044,8 +1044,10 @@ static int __do_replace(struct net *net, const char *name,
10441044

10451045
xt_free_table_info(oldinfo);
10461046
if (copy_to_user(counters_ptr, counters,
1047-
sizeof(struct xt_counters) * num_counters) != 0)
1048-
ret = -EFAULT;
1047+
sizeof(struct xt_counters) * num_counters) != 0) {
1048+
/* Silent error, can't fail, new table is already in place */
1049+
net_warn_ratelimited("arptables: counters copy to user failed while replacing table\n");
1050+
}
10491051
vfree(counters);
10501052
xt_table_unlock(t);
10511053
return ret;

net/ipv4/netfilter/ip_tables.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1231,8 +1231,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
12311231

12321232
xt_free_table_info(oldinfo);
12331233
if (copy_to_user(counters_ptr, counters,
1234-
sizeof(struct xt_counters) * num_counters) != 0)
1235-
ret = -EFAULT;
1234+
sizeof(struct xt_counters) * num_counters) != 0) {
1235+
/* Silent error, can't fail, new table is already in place */
1236+
net_warn_ratelimited("iptables: counters copy to user failed while replacing table\n");
1237+
}
12361238
vfree(counters);
12371239
xt_table_unlock(t);
12381240
return ret;

net/ipv6/netfilter/ip6_tables.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1241,8 +1241,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
12411241

12421242
xt_free_table_info(oldinfo);
12431243
if (copy_to_user(counters_ptr, counters,
1244-
sizeof(struct xt_counters) * num_counters) != 0)
1245-
ret = -EFAULT;
1244+
sizeof(struct xt_counters) * num_counters) != 0) {
1245+
/* Silent error, can't fail, new table is already in place */
1246+
net_warn_ratelimited("ip6tables: counters copy to user failed while replacing table\n");
1247+
}
12461248
vfree(counters);
12471249
xt_table_unlock(t);
12481250
return ret;

0 commit comments

Comments
 (0)