[PATCH 1/2] netfilter: nf_tables: do not allow NFT_SET_ELEM_INTERVAL_END flag and data

[PATCH 1/2] netfilter: nf_tables: do not allow NFT_SET_ELEM_INTERVAL_END flag and data

[Pablo Neira Ayuso]

This combination is not allowed since end interval elements cannot
contain data.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
Let's just make sure that userspace don't feed us with things that
we don't allow.

 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 3a2e480..d0c790e3e 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2741,6 +2741,9 @@ static int nft_add_set_elem(const struct nft_ctx *ctx, struct nft_set *set,
 		if (nla[NFTA_SET_ELEM_DATA] == NULL &&
 		    !(elem.flags & NFT_SET_ELEM_INTERVAL_END))
 			return -EINVAL;
+		if (nla[NFTA_SET_ELEM_DATA] != NULL &&
+		    elem.flags & NFT_SET_ELEM_INTERVAL_END)
+			return -EINVAL;
 	} else {
 		if (nla[NFTA_SET_ELEM_DATA] != NULL)
 			return -EINVAL;
-- 
1.7.10.4

[PATCH 2/2] netfilter: nft_rbtree: fix data handling of end interval elements

[Pablo Neira Ayuso]

This patch fixes several things which related to the handling of
end interval elements:

* Chain use underflow with intervals and map: If you add a rule
  using intervals+map that introduces a loop, the error path of the
  rbtree set decrements the chain refcount for each side of the
  interval, leading to a chain use counter underflow.

* Don't copy the data part of the end interval element since, this
  area is uninitialized and this confuses the loop detection code.

* Don't allocate room for the data part of end interval elements
  since this is unused.

So, after this patch the idea is that end interval elements don't
have a data part.

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
---
This patch extends http://patchwork.ozlabs.org/patch/317485/.

@Patrick, you mentioned also that nft_hash needs to be adjusted, but
after looking at this again I think there's no problem there since
hash cannot currently be selected for interval sets. Thanks for your
comments on the initial patch :)

 net/netfilter/nft_rbtree.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nft_rbtree.c b/net/netfilter/nft_rbtree.c
index ca0c1b2..e21d69d 100644
--- a/net/netfilter/nft_rbtree.c
+++ b/net/netfilter/nft_rbtree.c
@@ -69,8 +69,10 @@ static void nft_rbtree_elem_destroy(const struct nft_set *set,
 				    struct nft_rbtree_elem *rbe)
 {
 	nft_data_uninit(&rbe->key, NFT_DATA_VALUE);
-	if (set->flags & NFT_SET_MAP)
+	if (set->flags & NFT_SET_MAP &&
+	    !(rbe->flags & NFT_SET_ELEM_INTERVAL_END))
 		nft_data_uninit(rbe->data, set->dtype);
+
 	kfree(rbe);
 }
 
@@ -108,7 +110,8 @@ static int nft_rbtree_insert(const struct nft_set *set,
 	int err;
 
 	size = sizeof(*rbe);
-	if (set->flags & NFT_SET_MAP)
+	if (set->flags & NFT_SET_MAP &&
+	    !(elem->flags & NFT_SET_ELEM_INTERVAL_END))
 		size += sizeof(rbe->data[0]);
 
 	rbe = kzalloc(size, GFP_KERNEL);
@@ -117,7 +120,8 @@ static int nft_rbtree_insert(const struct nft_set *set,
 
 	rbe->flags = elem->flags;
 	nft_data_copy(&rbe->key, &elem->key);
-	if (set->flags & NFT_SET_MAP)
+	if (set->flags & NFT_SET_MAP &&
+	    !(rbe->flags & NFT_SET_ELEM_INTERVAL_END))
 		nft_data_copy(rbe->data, &elem->data);
 
 	err = __nft_rbtree_insert(set, rbe);
@@ -153,7 +157,8 @@ static int nft_rbtree_get(const struct nft_set *set, struct nft_set_elem *elem)
 			parent = parent->rb_right;
 		else {
 			elem->cookie = rbe;
-			if (set->flags & NFT_SET_MAP)
+			if (set->flags & NFT_SET_MAP &&
+			    !(rbe->flags & NFT_SET_ELEM_INTERVAL_END))
 				nft_data_copy(&elem->data, rbe->data);
 			elem->flags = rbe->flags;
 			return 0;
@@ -177,7 +182,8 @@ static void nft_rbtree_walk(const struct nft_ctx *ctx,
 
 		rbe = rb_entry(node, struct nft_rbtree_elem, node);
 		nft_data_copy(&elem.key, &rbe->key);
-		if (set->flags & NFT_SET_MAP)
+		if (set->flags & NFT_SET_MAP &&
+		    !(rbe->flags & NFT_SET_ELEM_INTERVAL_END))
 			nft_data_copy(&elem.data, rbe->data);
 		elem.flags = rbe->flags;
 
-- 
1.7.10.4

Re: [PATCH 2/2] netfilter: nft_rbtree: fix data handling of end interval elements

[Patrick McHardy]

On Fri, Feb 07, 2014 at 02:15:47PM +0100, Pablo Neira Ayuso wrote:
> This patch fixes several things which related to the handling of
> end interval elements:
> 
> * Chain use underflow with intervals and map: If you add a rule
>   using intervals+map that introduces a loop, the error path of the
>   rbtree set decrements the chain refcount for each side of the
>   interval, leading to a chain use counter underflow.
> 
> * Don't copy the data part of the end interval element since, this
>   area is uninitialized and this confuses the loop detection code.
> 
> * Don't allocate room for the data part of end interval elements
>   since this is unused.
> 
> So, after this patch the idea is that end interval elements don't
> have a data part.
> 
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> This patch extends http://patchwork.ozlabs.org/patch/317485/.
> 
> @Patrick, you mentioned also that nft_hash needs to be adjusted, but
> after looking at this again I think there's no problem there since
> hash cannot currently be selected for interval sets. Thanks for your
> comments on the initial patch :)


Correct, just noticed that myself :)

Acked-by: Patrick McHardy <kaber@trash.net>

for both patches.