[PATCH] netfilter: nf_tables: don't update chain with unset counters

[PATCH] netfilter: nf_tables: don't update chain with unset counters

[Pablo Neira Ayuso]

Fix possible replacemen of the per-cpu chain counters by null
pointer when updating an existing chain in the commit path.

Reported-by: Matteo Croce <technoboy85@gmail.com>
Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index f95dc95..f7dce2b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -899,6 +899,9 @@ static struct nft_stats __percpu *nft_stats_alloc(const struct nlattr *attr)
 static void nft_chain_stats_replace(struct nft_base_chain *chain,
 				    struct nft_stats __percpu *newstats)
 {
+	if (newstats == NULL)
+		return;
+
 	if (chain->stats) {
 		struct nft_stats __percpu *oldstats =
 				nft_dereference(chain->stats);
-- 
1.7.10.4

Re: [PATCH] netfilter: nf_tables: don't update chain with unset counters

[Eric Dumazet]

On Tue, 2014-08-05 at 17:30 +0200, Pablo Neira Ayuso wrote:
> Fix possible replacemen of the per-cpu chain counters by null
> pointer when updating an existing chain in the commit path.
> 
> Reported-by: Matteo Croce <technoboy85@gmail.com>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
>  net/netfilter/nf_tables_api.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index f95dc95..f7dce2b 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -899,6 +899,9 @@ static struct nft_stats __percpu *nft_stats_alloc(const struct nlattr *attr)
>  static void nft_chain_stats_replace(struct nft_base_chain *chain,
>  				    struct nft_stats __percpu *newstats)
>  {
> +	if (newstats == NULL)
> +		return;
> +
>  	if (chain->stats) {
>  		struct nft_stats __percpu *oldstats =
>  				nft_dereference(chain->stats);

This looks strange. Real bug is that nft_dump_stats() should not try to
fold percpu stats and output something if 'stats' is NULL ?

Otherwise, why nft_chain_stats_replace() checks if previous pointer is
NULL ?

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 93692d692ebc..9fbbb42dcffa 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -648,6 +648,9 @@ static int nft_dump_stats(struct sk_buff *skb, struct nft_stats __percpu *stats)
 	u64 pkts, bytes;
 	int cpu;
 
+	if (!stats)
+		return 0;
+
 	memset(&total, 0, sizeof(total));
 	for_each_possible_cpu(cpu) {
 		cpu_stats = per_cpu_ptr(stats, cpu);

Re: [PATCH] netfilter: nf_tables: don't update chain with unset counters

[Pablo Neira Ayuso]

On Tue, Aug 05, 2014 at 06:07:11PM +0200, Eric Dumazet wrote:
> On Tue, 2014-08-05 at 17:30 +0200, Pablo Neira Ayuso wrote:
> > Fix possible replacemen of the per-cpu chain counters by null
> > pointer when updating an existing chain in the commit path.
> > 
> > Reported-by: Matteo Croce <technoboy85@gmail.com>
> > Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> > ---
> >  net/netfilter/nf_tables_api.c |    3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index f95dc95..f7dce2b 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -899,6 +899,9 @@ static struct nft_stats __percpu *nft_stats_alloc(const struct nlattr *attr)
> >  static void nft_chain_stats_replace(struct nft_base_chain *chain,
> >  				    struct nft_stats __percpu *newstats)
> >  {
> > +	if (newstats == NULL)
> > +		return;
> > +
> >  	if (chain->stats) {
> >  		struct nft_stats __percpu *oldstats =
> >  				nft_dereference(chain->stats);
> 
> This looks strange. Real bug is that nft_dump_stats() should not try to
> fold percpu stats and output something if 'stats' is NULL ?
>
> Otherwise, why nft_chain_stats_replace() checks if previous pointer is
> NULL ?

Currently, we always allocate the per-cpu chain stats. The problem
occurs here:

static int nf_tables_newchain(struct sock *nlsk, ...
        ...
        if (chain != NULL) {
                struct nft_stats *stats = NULL;
                ...

                if (nla[NFTA_CHAIN_COUNTERS]) {
                        ...
                }
                ...
                nft_trans_chain_stats(trans) = stats;

Then, in the nf_tables_commit() path, this calls
nft_chain_commit_update() which sets the per-chain stats to NULL,
which is bad.

The idea is that, if no chain stats are passed when updating the
chain, this means: "please, leave the chain counter as is in this
update".

This bug slipped through with the transaction infrastructure.