[PATCH 0/3] netfilter: nf_tables fixes

[PATCH 0/3] netfilter: nf_tables fixes

[Patrick McHardy]

I'm starting to push over my nftables set patches, as a first
batch these three patches fix some bugs I noticed in the process:

* a race condition in the transaction code
* a possible rule dlen overflow
* a possible userdata ulen overflow

Since they will probably go through nf.git and I also require them
in nf-next.git for my following patches, I'd appreciate if you
could merge them into nf-next as soon as possible.

Thanks!


Patrick McHardy (3):
      netfilter: nf_tables: fix transaction race condition
      netfilter: nf_tables: check for overflow of rule dlen field
      netfilter: nf_tables: fix userdata length overflow

 include/net/netfilter/nf_tables.h | 22 +++++++++++++++++++---
 net/netfilter/nf_tables_api.c     | 34 ++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 13 deletions(-)

[PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Patrick McHardy]

A race condition exists in the rule transaction code for rules that
get added and removed within the same transaction.

The new rule starts out as inactive in the current and active in the
next generation and is inserted into the ruleset. When it is deleted,
it is additionally set to inactive in the next generation as well.

On commit the next generation is begun, then the actions are finalized.
For the new rule this would mean clearing out the inactive bit for
the previously current, now next generation.

However nft_rule_clear() clears out the bits for *both* generations,
activating the rule in the current generation, where it should be
deactivated due to being deleted. The rule will thus be active until
the deletion is finalized, removing the rule from the ruleset.

Similarly, when aborting a transaction for the same case, the undo
of insertion will remove it from the RCU protected rule list, the
deletion will clear out all bits. However until the next RCU
synchronization after all operations have been undone, the rule is
active on CPUs which can still see the rule on the list.

Generally, there may never be any modifications of the current
generations' inactive bit since this defeats the entire purpose of
atomicity. Change nft_rule_clear() to only touch the next generations
bit to fix this.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a8c9462..6fb532b 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -227,7 +227,7 @@ nft_rule_deactivate_next(struct net *net, struct nft_rule *rule)
 
 static inline void nft_rule_clear(struct net *net, struct nft_rule *rule)
 {
-	rule->genmask = 0;
+	rule->genmask &= ~(1 << gencursor_next(net));
 }
 
 static int
-- 
2.1.0

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Pablo Neira Ayuso]

On Tue, Mar 03, 2015 at 08:04:18PM +0000, Patrick McHardy wrote:
> A race condition exists in the rule transaction code for rules that
> get added and removed within the same transaction.
> 
> The new rule starts out as inactive in the current and active in the
> next generation and is inserted into the ruleset. When it is deleted,
> it is additionally set to inactive in the next generation as well.
> 
> On commit the next generation is begun, then the actions are finalized.
> For the new rule this would mean clearing out the inactive bit for
> the previously current, now next generation.
> 
> However nft_rule_clear() clears out the bits for *both* generations,
> activating the rule in the current generation, where it should be
> deactivated due to being deleted. The rule will thus be active until
> the deletion is finalized, removing the rule from the ruleset.
> 
> Similarly, when aborting a transaction for the same case, the undo
> of insertion will remove it from the RCU protected rule list, the
> deletion will clear out all bits. However until the next RCU
> synchronization after all operations have been undone, the rule is
> active on CPUs which can still see the rule on the list.
> 
> Generally, there may never be any modifications of the current
> generations' inactive bit since this defeats the entire purpose of
> atomicity. Change nft_rule_clear() to only touch the next generations
> bit to fix this.

I think we can get rid of the nft_rule_clear() call from the error
path of nf_tables_newrule() too.

> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---
>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index a8c9462..6fb532b 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -227,7 +227,7 @@ nft_rule_deactivate_next(struct net *net, struct nft_rule *rule)
>  
>  static inline void nft_rule_clear(struct net *net, struct nft_rule *rule)
>  {
> -	rule->genmask = 0;
> +	rule->genmask &= ~(1 << gencursor_next(net));
>  }
>  
>  static int
> -- 
> 2.1.0
> 

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Patrick McHardy]

On 04.03, Pablo Neira Ayuso wrote:
> On Tue, Mar 03, 2015 at 08:04:18PM +0000, Patrick McHardy wrote:
> > A race condition exists in the rule transaction code for rules that
> > get added and removed within the same transaction.
> > 
> > The new rule starts out as inactive in the current and active in the
> > next generation and is inserted into the ruleset. When it is deleted,
> > it is additionally set to inactive in the next generation as well.
> > 
> > On commit the next generation is begun, then the actions are finalized.
> > For the new rule this would mean clearing out the inactive bit for
> > the previously current, now next generation.
> > 
> > However nft_rule_clear() clears out the bits for *both* generations,
> > activating the rule in the current generation, where it should be
> > deactivated due to being deleted. The rule will thus be active until
> > the deletion is finalized, removing the rule from the ruleset.
> > 
> > Similarly, when aborting a transaction for the same case, the undo
> > of insertion will remove it from the RCU protected rule list, the
> > deletion will clear out all bits. However until the next RCU
> > synchronization after all operations have been undone, the rule is
> > active on CPUs which can still see the rule on the list.
> > 
> > Generally, there may never be any modifications of the current
> > generations' inactive bit since this defeats the entire purpose of
> > atomicity. Change nft_rule_clear() to only touch the next generations
> > bit to fix this.
> 
> I think we can get rid of the nft_rule_clear() call from the error
> path of nf_tables_newrule() too.

I don't think so, we deactivate the old rule for NLM_F_REPLACE and
need to undo that on error.

Or are you talking about getting rid of the entire error handling
for NLM_F_REPLACE and have it taken care of by the abort() path?

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Pablo Neira Ayuso]

[-- Attachment #1: Type: text/plain, Size: 2120 bytes --]

On Wed, Mar 04, 2015 at 04:26:13PM +0000, Patrick McHardy wrote:
> On 04.03, Pablo Neira Ayuso wrote:
> > On Tue, Mar 03, 2015 at 08:04:18PM +0000, Patrick McHardy wrote:
> > > A race condition exists in the rule transaction code for rules that
> > > get added and removed within the same transaction.
> > > 
> > > The new rule starts out as inactive in the current and active in the
> > > next generation and is inserted into the ruleset. When it is deleted,
> > > it is additionally set to inactive in the next generation as well.
> > > 
> > > On commit the next generation is begun, then the actions are finalized.
> > > For the new rule this would mean clearing out the inactive bit for
> > > the previously current, now next generation.
> > > 
> > > However nft_rule_clear() clears out the bits for *both* generations,
> > > activating the rule in the current generation, where it should be
> > > deactivated due to being deleted. The rule will thus be active until
> > > the deletion is finalized, removing the rule from the ruleset.
> > > 
> > > Similarly, when aborting a transaction for the same case, the undo
> > > of insertion will remove it from the RCU protected rule list, the
> > > deletion will clear out all bits. However until the next RCU
> > > synchronization after all operations have been undone, the rule is
> > > active on CPUs which can still see the rule on the list.
> > > 
> > > Generally, there may never be any modifications of the current
> > > generations' inactive bit since this defeats the entire purpose of
> > > atomicity. Change nft_rule_clear() to only touch the next generations
> > > bit to fix this.
> > 
> > I think we can get rid of the nft_rule_clear() call from the error
> > path of nf_tables_newrule() too.
> 
> I don't think so, we deactivate the old rule for NLM_F_REPLACE and
> need to undo that on error.

Right.

> Or are you talking about getting rid of the entire error handling
> for NLM_F_REPLACE and have it taken care of by the abort() path?

Yes, that error handling I think we can get rid of it. It's actually
not correct because it's deleting the old rule.

[-- Attachment #2: 0001-netfilter-nf_tables-fix-error-handling-of-rule-repla.patch --]
[-- Type: text/x-diff, Size: 1255 bytes --]

>From f4ab0cab91e2968652745dc883d46da61421f560 Mon Sep 17 00:00:00 2001
From: Pablo Neira Ayuso <pablo@netfilter.org>
Date: Wed, 4 Mar 2015 17:55:27 +0100
Subject: [PATCH] netfilter: nf_tables: fix error handling of rule replacement

In general, if a transaction object is added to the list successfully,
we can rely on the abort path to undo what we've done. This allows us to
simplify the error handling of the rule replacement path in
nf_tables_newrule().

This implicitly fixes an unnecessary removal of the old rule removal,
which needs to be left in place if we fail to replace.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
 net/netfilter/nf_tables_api.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a8c9462..6668adb 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2031,12 +2031,6 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 
 err3:
 	list_del_rcu(&rule->list);
-	if (trans) {
-		list_del_rcu(&nft_trans_rule(trans)->list);
-		nft_rule_clear(net, nft_trans_rule(trans));
-		nft_trans_destroy(trans);
-		chain->use++;
-	}
 err2:
 	nf_tables_rule_destroy(&ctx, rule);
 err1:
-- 
1.7.10.4

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Patrick McHardy]

On 04.03, Pablo Neira Ayuso wrote:
> > > I think we can get rid of the nft_rule_clear() call from the error
> > > path of nf_tables_newrule() too.
> > 
> > I don't think so, we deactivate the old rule for NLM_F_REPLACE and
> > need to undo that on error.
> 
> Right.
> 
> > Or are you talking about getting rid of the entire error handling
> > for NLM_F_REPLACE and have it taken care of by the abort() path?
> 
> Yes, that error handling I think we can get rid of it. It's actually
> not correct because it's deleting the old rule.

It does? All I can see is reactivating it?

> In general, if a transaction object is added to the list successfully,
> we can rely on the abort path to undo what we've done. This allows us to
> simplify the error handling of the rule replacement path in
> nf_tables_newrule().
> 
> This implicitly fixes an unnecessary removal of the old rule removal,
> which needs to be left in place if we fail to replace.

I agree on the simplification, but I don't see any problem with this.

>  err3:
>  	list_del_rcu(&rule->list);
> -	if (trans) {
> -		list_del_rcu(&nft_trans_rule(trans)->list);
> -		nft_rule_clear(net, nft_trans_rule(trans));
> -		nft_trans_destroy(trans);
> -		chain->use++;
> -	}
>  err2:
>  	nf_tables_rule_destroy(&ctx, rule);
>  err1:
> -- 
> 1.7.10.4
> 

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Pablo Neira Ayuso]

On Wed, Mar 04, 2015 at 05:03:56PM +0000, Patrick McHardy wrote:
> On 04.03, Pablo Neira Ayuso wrote:
> > > > I think we can get rid of the nft_rule_clear() call from the error
> > > > path of nf_tables_newrule() too.
> > > 
> > > I don't think so, we deactivate the old rule for NLM_F_REPLACE and
> > > need to undo that on error.
> > 
> > Right.
> > 
> > > Or are you talking about getting rid of the entire error handling
> > > for NLM_F_REPLACE and have it taken care of by the abort() path?
> > 
> > Yes, that error handling I think we can get rid of it. It's actually
> > not correct because it's deleting the old rule.
> 
> It does? All I can see is reactivating it?
> 
> > In general, if a transaction object is added to the list successfully,
> > we can rely on the abort path to undo what we've done. This allows us to
> > simplify the error handling of the rule replacement path in
> > nf_tables_newrule().
> > 
> > This implicitly fixes an unnecessary removal of the old rule removal,
> > which needs to be left in place if we fail to replace.
> 
> I agree on the simplification, but I don't see any problem with this.

Let me see, from the replacement path:

       trans = nft_trans_rule_add(&ctx, NFT_MSG_DELRULE,
                                  old_rule);
       ...
       nft_rule_deactivate_next(net, old_rule);
       chain->use--;
       list_add_tail_rcu(&rule->list, &old_rule->list);

So we basically:

1) add transaction object to delete the old rule
2) deactivate the old rule
3) reduce the chain use counter
4) add the new rule after the old rule

Then, if we fail to add the transaction for the new rule, what we have
in err3 says:

1) We remove the new rule from the chain, we couldn't add a
   transaction object for this, so we have to manually undo this.
2) We remove the old rule (but it should actually be left there in
   place).
3) Clear the old rule generation bits, as it needs to be active in the
   next generation given that we failed (this undoes step2)
4) Release the transaction object.
5) Restore chain use counter.

#3, #4 and #5 can be handled from the abort path.

#2 should not be there I think.

> >  err3:
> >  	list_del_rcu(&rule->list);
> > -	if (trans) {
> > -		list_del_rcu(&nft_trans_rule(trans)->list);
> > -		nft_rule_clear(net, nft_trans_rule(trans));
> > -		nft_trans_destroy(trans);
> > -		chain->use++;
> > -	}
> >  err2:
> >  	nf_tables_rule_destroy(&ctx, rule);
> >  err1:
> > -- 
> > 1.7.10.4
> > 
> 

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Patrick McHardy]

On 04.03, Pablo Neira Ayuso wrote:
> On Wed, Mar 04, 2015 at 05:03:56PM +0000, Patrick McHardy wrote:
> > On 04.03, Pablo Neira Ayuso wrote:
> > > > > I think we can get rid of the nft_rule_clear() call from the error
> > > > > path of nf_tables_newrule() too.
> > > > 
> > > > I don't think so, we deactivate the old rule for NLM_F_REPLACE and
> > > > need to undo that on error.
> > > 
> > > Right.
> > > 
> > > > Or are you talking about getting rid of the entire error handling
> > > > for NLM_F_REPLACE and have it taken care of by the abort() path?
> > > 
> > > Yes, that error handling I think we can get rid of it. It's actually
> > > not correct because it's deleting the old rule.
> > 
> > It does? All I can see is reactivating it?
> > 
> > > In general, if a transaction object is added to the list successfully,
> > > we can rely on the abort path to undo what we've done. This allows us to
> > > simplify the error handling of the rule replacement path in
> > > nf_tables_newrule().
> > > 
> > > This implicitly fixes an unnecessary removal of the old rule removal,
> > > which needs to be left in place if we fail to replace.
> > 
> > I agree on the simplification, but I don't see any problem with this.
> 
> Let me see, from the replacement path:
> 
>        trans = nft_trans_rule_add(&ctx, NFT_MSG_DELRULE,
>                                   old_rule);
>        ...
>        nft_rule_deactivate_next(net, old_rule);
>        chain->use--;
>        list_add_tail_rcu(&rule->list, &old_rule->list);
> 
> So we basically:
> 
> 1) add transaction object to delete the old rule
> 2) deactivate the old rule
> 3) reduce the chain use counter
> 4) add the new rule after the old rule
> 
> Then, if we fail to add the transaction for the new rule, what we have
> in err3 says:
> 
> 1) We remove the new rule from the chain, we couldn't add a
>    transaction object for this, so we have to manually undo this.
> 2) We remove the old rule (but it should actually be left there in
>    place).
> 3) Clear the old rule generation bits, as it needs to be active in the
>    next generation given that we failed (this undoes step2)
> 4) Release the transaction object.
> 5) Restore chain use counter.
> 
> #3, #4 and #5 can be handled from the abort path.
> 
> #2 should not be there I think.

I still don't see where we remove the old rule. We activate it
and remove the transaction object, but that's it.

Where do you see this?

Re: [PATCH 1/3] netfilter: nf_tables: fix transaction race condition

[Patrick McHardy]

On 04.03, Patrick McHardy wrote:
> On 04.03, Pablo Neira Ayuso wrote:
> > On Wed, Mar 04, 2015 at 05:03:56PM +0000, Patrick McHardy wrote:
> > > 
> > > I agree on the simplification, but I don't see any problem with this.
> > 
> > Let me see, from the replacement path:
> > 
> >        trans = nft_trans_rule_add(&ctx, NFT_MSG_DELRULE,
> >                                   old_rule);
> >        ...
> >        nft_rule_deactivate_next(net, old_rule);
> >        chain->use--;
> >        list_add_tail_rcu(&rule->list, &old_rule->list);
> > 
> > So we basically:
> > 
> > 1) add transaction object to delete the old rule
> > 2) deactivate the old rule
> > 3) reduce the chain use counter
> > 4) add the new rule after the old rule
> > 
> > Then, if we fail to add the transaction for the new rule, what we have
> > in err3 says:
> > 
> > 1) We remove the new rule from the chain, we couldn't add a
> >    transaction object for this, so we have to manually undo this.
> > 2) We remove the old rule (but it should actually be left there in
> >    place).
> > 3) Clear the old rule generation bits, as it needs to be active in the
> >    next generation given that we failed (this undoes step2)
> > 4) Release the transaction object.
> > 5) Restore chain use counter.
> > 
> > #3, #4 and #5 can be handled from the abort path.
> > 
> > #2 should not be there I think.
> 
> I still don't see where we remove the old rule. We activate it
> and remove the transaction object, but that's it.
> 
> Where do you see this?

Ok got it, I misread the code and through we'd only delete the
transaction object. Ok, so I agree that this actually fixes a bug :)

[PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Patrick McHardy]

Check that the space required for the expressions doesn't exceed the
size of the dlen field, which would lead to the iterators crashing.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 net/netfilter/nf_tables_api.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 6fb532b..7baafd5 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 			n++;
 		}
 	}
+	/* Check for overflow of dlen field */
+	err = -EFBIG;
+	if (size >= 1 << 12)
+		goto err1;
 
 	if (nla[NFTA_RULE_USERDATA])
 		ulen = nla_len(nla[NFTA_RULE_USERDATA]);
-- 
2.1.0

Re: [PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Pablo Neira Ayuso]

On Tue, Mar 03, 2015 at 08:04:19PM +0000, Patrick McHardy wrote:
> Check that the space required for the expressions doesn't exceed the
> size of the dlen field, which would lead to the iterators crashing.
> 
> Signed-off-by: Patrick McHardy <kaber@trash.net>
> ---
>  net/netfilter/nf_tables_api.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index 6fb532b..7baafd5 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
>  			n++;
>  		}
>  	}
> +	/* Check for overflow of dlen field */
> +	err = -EFBIG;

Should we use -EOVERFLOW instead as we use in other nf_tables spots?

The error in userspace will be: "Value too large for defined data type".

> +	if (size >= 1 << 12)
> +		goto err1;
>  
>  	if (nla[NFTA_RULE_USERDATA])
>  		ulen = nla_len(nla[NFTA_RULE_USERDATA]);
> -- 
> 2.1.0
> 

Re: [PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Patrick McHardy]

On 05.03, Pablo Neira Ayuso wrote:
> On Tue, Mar 03, 2015 at 08:04:19PM +0000, Patrick McHardy wrote:
> > Check that the space required for the expressions doesn't exceed the
> > size of the dlen field, which would lead to the iterators crashing.
> > 
> > Signed-off-by: Patrick McHardy <kaber@trash.net>
> > ---
> >  net/netfilter/nf_tables_api.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > index 6fb532b..7baafd5 100644
> > --- a/net/netfilter/nf_tables_api.c
> > +++ b/net/netfilter/nf_tables_api.c
> > @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
> >  			n++;
> >  		}
> >  	}
> > +	/* Check for overflow of dlen field */
> > +	err = -EFBIG;
> 
> Should we use -EOVERFLOW instead as we use in other nf_tables spots?
> 
> The error in userspace will be: "Value too large for defined data type".

I think the difference here is that we don't use a userspace provided
value but overflow because of the size of kernel internal structures and
the data type limit.

I don't have any strong feelings either way, but I think its different
from the cases where we use EOVERFLOW so far.

Re: [PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Pablo Neira Ayuso]

On Thu, Mar 05, 2015 at 04:31:22PM +0000, Patrick McHardy wrote:
> On 05.03, Pablo Neira Ayuso wrote:
> > On Tue, Mar 03, 2015 at 08:04:19PM +0000, Patrick McHardy wrote:
> > > Check that the space required for the expressions doesn't exceed the
> > > size of the dlen field, which would lead to the iterators crashing.
> > > 
> > > Signed-off-by: Patrick McHardy <kaber@trash.net>
> > > ---
> > >  net/netfilter/nf_tables_api.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> > > index 6fb532b..7baafd5 100644
> > > --- a/net/netfilter/nf_tables_api.c
> > > +++ b/net/netfilter/nf_tables_api.c
> > > @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
> > >  			n++;
> > >  		}
> > >  	}
> > > +	/* Check for overflow of dlen field */
> > > +	err = -EFBIG;
> > 
> > Should we use -EOVERFLOW instead as we use in other nf_tables spots?
> > 
> > The error in userspace will be: "Value too large for defined data type".
> 
> I think the difference here is that we don't use a userspace provided
> value but overflow because of the size of kernel internal structures and
> the data type limit.

OK, we also have it when we pass too large amount of data for a
register IIRC.

> I don't have any strong feelings either way, but I think its different
> from the cases where we use EOVERFLOW so far.

I don't have any strong opinion, just asking. You know we shouldn't
change this afterwards.

Let me know.

Re: [PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Patrick McHardy]

On 05.03, Pablo Neira Ayuso wrote:
> > > Should we use -EOVERFLOW instead as we use in other nf_tables spots?
> > > 
> > > The error in userspace will be: "Value too large for defined data type".
> > 
> > I think the difference here is that we don't use a userspace provided
> > value but overflow because of the size of kernel internal structures and
> > the data type limit.
> 
> OK, we also have it when we pass too large amount of data for a
> register IIRC.

Sure. But that's data we pass, in that size. In this case its data
the kernel allocates, the amount of which is defined purely by
the kernel.

> > I don't have any strong feelings either way, but I think its different
> > from the cases where we use EOVERFLOW so far.
> 
> I don't have any strong opinion, just asking. You know we shouldn't
> change this afterwards.
> 
> Let me know.

I think this case shouldn't exist at all since it codifies kernel
internals into the API. Today we might accept 128 expressions, the
other day just 100, all depending on 32 or 64 bit systems. Its not
good.

The problem is I also don't want to increase the maximum rule size
to more than 4k since this will run into issues with memory
fragmentation. I think we need to decrease NFT_EXPR_MAXNUM, its
unreasonable large, and then add a per expression maximum size.

For now I'd say apply it as it is, keep the different errno code
since its a really different case, and we'll fix up the underlying
problem after a bit more of thought.

Re: [PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Pablo Neira Ayuso]

On Thu, Mar 05, 2015 at 05:13:53PM +0000, Patrick McHardy wrote:
> I think this case shouldn't exist at all since it codifies kernel
> internals into the API. Today we might accept 128 expressions, the
> other day just 100, all depending on 32 or 64 bit systems. Its not
> good.
> 
> The problem is I also don't want to increase the maximum rule size
> to more than 4k since this will run into issues with memory
> fragmentation. I think we need to decrease NFT_EXPR_MAXNUM, its
> unreasonable large, and then add a per expression maximum size.

Yes, that's more than anyone would need I guess. We used to have a
couple of crazy tests in iptables with lots of matches in one rule
(maps to one expression each) that were hitting the limit. So my
motivation was to avoid hitting that it from nft_compat. Assuming that
the info area consumes ~32 bytes and 128 expressions, we would exactly
reach the limit. But there are xtables extensions consuming more than
that for the info area, so we may hit the limit before those 128
expressions with this check. I think that's OK, we should consider
realistic scenarios, not too crazy tests.

> For now I'd say apply it as it is, keep the different errno code
> since its a really different case, and we'll fix up the underlying
> problem after a bit more of thought.

Sure, thanks for the explanation.

Re: [PATCH 2/3] netfilter: nf_tables: check for overflow of rule dlen field

[Patrick McHardy]

On 05.03, Pablo Neira Ayuso wrote:
> On Thu, Mar 05, 2015 at 05:13:53PM +0000, Patrick McHardy wrote:
> > I think this case shouldn't exist at all since it codifies kernel
> > internals into the API. Today we might accept 128 expressions, the
> > other day just 100, all depending on 32 or 64 bit systems. Its not
> > good.
> > 
> > The problem is I also don't want to increase the maximum rule size
> > to more than 4k since this will run into issues with memory
> > fragmentation. I think we need to decrease NFT_EXPR_MAXNUM, its
> > unreasonable large, and then add a per expression maximum size.
> 
> Yes, that's more than anyone would need I guess. We used to have a
> couple of crazy tests in iptables with lots of matches in one rule
> (maps to one expression each) that were hitting the limit. So my
> motivation was to avoid hitting that it from nft_compat. Assuming that
> the info area consumes ~32 bytes and 128 expressions, we would exactly
> reach the limit. But there are xtables extensions consuming more than
> that for the info area, so we may hit the limit before those 128
> expressions with this check. I think that's OK, we should consider
> realistic scenarios, not too crazy tests.

Agreed, it doesn't really matter. But I still believe the API should
not potentially fail because of kernel internal changes. I'd rather
have strict limits that kick in *before* we use up all our space and
those crazy test cases simply won't work with nftables.

> > For now I'd say apply it as it is, keep the different errno code
> > since its a really different case, and we'll fix up the underlying
> > problem after a bit more of thought.
> 
> Sure, thanks for the explanation.

[PATCH 3/3] netfilter: nf_tables: fix userdata length overflow

[Patrick McHardy]

The NFT_USERDATA_MAXLEN is defined to 256, however we only have a u8
to store its size. Introduce a struct nft_userdata which contains a
length field and indicate its presence using a single bit in the rule.

The length field of struct nft_userdata is also a u8, however we don't
store zero sized data, so the actual length is udata->len + 1.

Signed-off-by: Patrick McHardy <kaber@trash.net>
---
 include/net/netfilter/nf_tables.h | 22 +++++++++++++++++++---
 net/netfilter/nf_tables_api.c     | 28 +++++++++++++++++++---------
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 9eaaa78..decb9a0 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -119,6 +119,22 @@ int nft_validate_data_load(const struct nft_ctx *ctx, enum nft_registers reg,
 			   const struct nft_data *data,
 			   enum nft_data_types type);
 
+
+/**
+ *	struct nft_userdata - user defined data associated with an object
+ *
+ *	@len: length of the data
+ *	@data: content
+ *
+ *	The presence of user data is indicated in an object specific fashion,
+ *	so a length of zero can't occur and the value "len" indicates data
+ *	of length len + 1.
+ */
+struct nft_userdata {
+	u8			len;
+	unsigned char		data[0];
+};
+
 /**
  *	struct nft_set_elem - generic representation of set elements
  *
@@ -380,7 +396,7 @@ static inline void *nft_expr_priv(const struct nft_expr *expr)
  *	@handle: rule handle
  *	@genmask: generation mask
  *	@dlen: length of expression data
- *	@ulen: length of user data (used for comments)
+ *	@udata: user data is appended to the rule
  *	@data: expression data
  */
 struct nft_rule {
@@ -388,7 +404,7 @@ struct nft_rule {
 	u64				handle:42,
 					genmask:2,
 					dlen:12,
-					ulen:8;
+					udata:1;
 	unsigned char			data[]
 		__attribute__((aligned(__alignof__(struct nft_expr))));
 };
@@ -476,7 +492,7 @@ static inline struct nft_expr *nft_expr_last(const struct nft_rule *rule)
 	return (struct nft_expr *)&rule->data[rule->dlen];
 }
 
-static inline void *nft_userdata(const struct nft_rule *rule)
+static inline struct nft_userdata *nft_userdata(const struct nft_rule *rule)
 {
 	return (void *)&rule->data[rule->dlen];
 }
diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index 7baafd5..74e4b87 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -1711,9 +1711,12 @@ static int nf_tables_fill_rule_info(struct sk_buff *skb, struct net *net,
 	}
 	nla_nest_end(skb, list);
 
-	if (rule->ulen &&
-	    nla_put(skb, NFTA_RULE_USERDATA, rule->ulen, nft_userdata(rule)))
-		goto nla_put_failure;
+	if (rule->udata) {
+		struct nft_userdata *udata = nft_userdata(rule);
+		if (nla_put(skb, NFTA_RULE_USERDATA, udata->len + 1,
+			    udata->data) < 0)
+			goto nla_put_failure;
+	}
 
 	nlmsg_end(skb, nlh);
 	return 0;
@@ -1896,11 +1899,12 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 	struct nft_table *table;
 	struct nft_chain *chain;
 	struct nft_rule *rule, *old_rule = NULL;
+	struct nft_userdata *udata;
 	struct nft_trans *trans = NULL;
 	struct nft_expr *expr;
 	struct nft_ctx ctx;
 	struct nlattr *tmp;
-	unsigned int size, i, n, ulen = 0;
+	unsigned int size, i, n, ulen = 0, usize = 0;
 	int err, rem;
 	bool create;
 	u64 handle, pos_handle;
@@ -1973,11 +1977,14 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 	if (size >= 1 << 12)
 		goto err1;
 
-	if (nla[NFTA_RULE_USERDATA])
+	if (nla[NFTA_RULE_USERDATA]) {
 		ulen = nla_len(nla[NFTA_RULE_USERDATA]);
+		if (ulen > 0)
+			usize = sizeof(struct nft_userdata) + ulen;
+	}
 
 	err = -ENOMEM;
-	rule = kzalloc(sizeof(*rule) + size + ulen, GFP_KERNEL);
+	rule = kzalloc(sizeof(*rule) + size + usize, GFP_KERNEL);
 	if (rule == NULL)
 		goto err1;
 
@@ -1985,10 +1992,13 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb,
 
 	rule->handle = handle;
 	rule->dlen   = size;
-	rule->ulen   = ulen;
+	rule->udata  = ulen ? 1 : 0;
 
-	if (ulen)
-		nla_memcpy(nft_userdata(rule), nla[NFTA_RULE_USERDATA], ulen);
+	if (ulen) {
+		udata = nft_userdata(rule);
+		udata->len = ulen - 1;
+		nla_memcpy(udata->data, nla[NFTA_RULE_USERDATA], ulen);
+	}
 
 	expr = nft_expr_first(rule);
 	for (i = 0; i < n; i++) {
-- 
2.1.0

Re: [PATCH 0/3] netfilter: nf_tables fixes

[Pablo Neira Ayuso]

On Tue, Mar 03, 2015 at 08:04:17PM +0000, Patrick McHardy wrote:
> I'm starting to push over my nftables set patches, as a first
> batch these three patches fix some bugs I noticed in the process:
> 
> * a race condition in the transaction code
> * a possible rule dlen overflow
> * a possible userdata ulen overflow

Applied, thanks a lot Patrick.