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>

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 0e474b13463b95a90a185f883d3435db346efa34..1059ed3bc2557d597cb0548962a888e544d74a67 100644 (file)
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1044,10 +1044,9 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
        if (repl->num_counters &&
           copy_to_user(repl->counters, counterstmp,
           repl->num_counters * sizeof(struct ebt_counter))) {
-               ret = -EFAULT;
+               /* Silent error, can't fail, new table is already in place */
+               net_warn_ratelimited("ebtables: counters copy to user failed while replacing table\n");
        }
-       else
-               ret = 0;
 
        /* decrease module count and free resources */
        EBT_ENTRY_ITERATE(table->entries, table->entries_size,

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 59da7cde072447c2331422659adb7dadad4f3fae..f95b6f93814b95b2c810eff8d4573a996f9a9f63 100644 (file)
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -1044,8 +1044,10 @@ static int __do_replace(struct net *net, const char *name,
 
        xt_free_table_info(oldinfo);
        if (copy_to_user(counters_ptr, counters,
-                        sizeof(struct xt_counters) * num_counters) != 0)
-               ret = -EFAULT;
+                        sizeof(struct xt_counters) * num_counters) != 0) {
+               /* Silent error, can't fail, new table is already in place */
+               net_warn_ratelimited("arptables: counters copy to user failed while replacing table\n");
+       }
        vfree(counters);
        xt_table_unlock(t);
        return ret;

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 718dfbd30cbe09560c1d545525b446fb5695f6af..99e810f84671bbdb80e33c6915cc7be49ba0f1bb 100644 (file)
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1231,8 +1231,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 
        xt_free_table_info(oldinfo);
        if (copy_to_user(counters_ptr, counters,
-                        sizeof(struct xt_counters) * num_counters) != 0)
-               ret = -EFAULT;
+                        sizeof(struct xt_counters) * num_counters) != 0) {
+               /* Silent error, can't fail, new table is already in place */
+               net_warn_ratelimited("iptables: counters copy to user failed while replacing table\n");
+       }
        vfree(counters);
        xt_table_unlock(t);
        return ret;

diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 710238f58aa93c0319cf90adb0c203c35f8bc7e4..e080fbbbc0e5ce8d4d71014eed149e6b34250b62 100644 (file)
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1241,8 +1241,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
 
        xt_free_table_info(oldinfo);
        if (copy_to_user(counters_ptr, counters,
-                        sizeof(struct xt_counters) * num_counters) != 0)
-               ret = -EFAULT;
+                        sizeof(struct xt_counters) * num_counters) != 0) {
+               /* Silent error, can't fail, new table is already in place */
+               net_warn_ratelimited("ip6tables: counters copy to user failed while replacing table\n");
+       }
        vfree(counters);
        xt_table_unlock(t);
        return ret;