[PATCH] netfilter: Can't fail and free after table replacement

[PATCH] netfilter: Can't fail and free after table replacement

[Thomas Graf]

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>
---
 net/bridge/netfilter/ebtables.c | 4 +---
 net/ipv4/netfilter/arp_tables.c | 5 +++--
 net/ipv4/netfilter/ip_tables.c  | 5 +++--
 net/ipv6/netfilter/ip6_tables.c | 5 +++--
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 0e474b1..7a3dc98 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -1044,10 +1044,8 @@ 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 */
 	}
-	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 59da7cd..4bf025f 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -1044,8 +1044,9 @@ 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 */
+	}
 	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 718dfbd..02d5566 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -1231,8 +1231,9 @@ __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 */
+	}
 	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 710238f..24dc908 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -1241,8 +1241,9 @@ __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 */
+	}
 	vfree(counters);
 	xt_table_unlock(t);
 	return ret;
-- 
1.8.3.1

Re: [PATCH] netfilter: Can't fail and free after table replacement

[Pablo Neira Ayuso]

On Wed, Apr 02, 2014 at 05:35:13PM +0200, Thomas Graf wrote:
> 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>
> ---
>  net/bridge/netfilter/ebtables.c | 4 +---
>  net/ipv4/netfilter/arp_tables.c | 5 +++--
>  net/ipv4/netfilter/ip_tables.c  | 5 +++--
>  net/ipv6/netfilter/ip6_tables.c | 5 +++--
>  4 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index 0e474b1..7a3dc98 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -1044,10 +1044,8 @@ 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 */
>  	}
> -	else
> -		ret = 0;
>  

This seems good to me.

Perhaps we can spot a warning like in rtnetlink to inform the user
that counters are not reliable anymore?

Re: [PATCH] netfilter: Can't fail and free after table replacement

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Wed, Apr 02, 2014 at 05:35:13PM +0200, Thomas Graf wrote:
> > 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>
> > ---
> >  net/bridge/netfilter/ebtables.c | 4 +---
> >  net/ipv4/netfilter/arp_tables.c | 5 +++--
> >  net/ipv4/netfilter/ip_tables.c  | 5 +++--
> >  net/ipv6/netfilter/ip6_tables.c | 5 +++--
> >  4 files changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> > index 0e474b1..7a3dc98 100644
> > --- a/net/bridge/netfilter/ebtables.c
> > +++ b/net/bridge/netfilter/ebtables.c
> > @@ -1044,10 +1044,8 @@ 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 */
> >  	}
> > -	else
> > -		ret = 0;
> >  
> 
> This seems good to me.
> 
> Perhaps we can spot a warning like in rtnetlink to inform the user
> that counters are not reliable anymore?

you mean net_warn_ratelimit() ?

Sure, can be added.

However given that this bug has been around for 9 years I don't think
its really needed, if it fails kernel panic'd, so its safe to say
that the counters are reliable ;)

With vanilla iptables this only fails if the userspace buffer was
swapped out and could not be paged back in due to OOM.

Re: [PATCH] netfilter: Can't fail and free after table replacement

[Pablo Neira Ayuso]

On Fri, Apr 04, 2014 at 12:08:26AM +0200, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Wed, Apr 02, 2014 at 05:35:13PM +0200, Thomas Graf wrote:
> > > 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>
> > > ---
> > >  net/bridge/netfilter/ebtables.c | 4 +---
> > >  net/ipv4/netfilter/arp_tables.c | 5 +++--
> > >  net/ipv4/netfilter/ip_tables.c  | 5 +++--
> > >  net/ipv6/netfilter/ip6_tables.c | 5 +++--
> > >  4 files changed, 10 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> > > index 0e474b1..7a3dc98 100644
> > > --- a/net/bridge/netfilter/ebtables.c
> > > +++ b/net/bridge/netfilter/ebtables.c
> > > @@ -1044,10 +1044,8 @@ 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 */
> > >  	}
> > > -	else
> > > -		ret = 0;
> > >  
> > 
> > This seems good to me.
> > 
> > Perhaps we can spot a warning like in rtnetlink to inform the user
> > that counters are not reliable anymore?
> 
> you mean net_warn_ratelimit() ?
> 
> Sure, can be added.
> 
> However given that this bug has been around for 9 years I don't think
> its really needed, if it fails kernel panic'd, so its safe to say
> that the counters are reliable ;)

But we are not crashing anymore, right? That swapped out scenario may
happen in a short-time stress situation from the memory POV. Let's
just be informative, it's just one extra line ahead.