netfilter 00/03: netfilter fixes

netfilter 00/03: netfilter fixes

[Patrick McHardy]

Hi Dave,

following are three netfilter fixes for net-next, fixing:

- the NAT issue reported by Stephen, which was caused by inverted logic
  in NF_HOOK_COND(), causing it to skip the POST_ROUTING hook invocation

- an assertion in ct_extend, caused by invalid ordering in ctnetlink
  when setting up new conntracks. Additionally it is invalid to
  attach helpers to existing conntracks, which is disabled by this
  patch.

- an skb leak in nf_queue when userspace returns NF_STOLEN as verdict

Please apply or pull from:

git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6.git master

Thanks!


 include/linux/netfilter.h            |    5 +++--
 net/netfilter/nf_conntrack_netlink.c |   22 +++++++++++-----------
 net/netfilter/nf_queue.c             |    2 +-
 3 files changed, 15 insertions(+), 14 deletions(-)

Eric Dumazet (1):
      netfilter: nf_queue: fix NF_STOLEN skb leak

Pablo Neira Ayuso (1):
      netfilter: ctnetlink: fix creation of conntrack with helpers

Patrick McHardy (1):
      netfilter: restore POST_ROUTING hook in NF_HOOK_COND

netfilter 01/03: restore POST_ROUTING hook in NF_HOOK_COND

[Patrick McHardy]

commit 4bac6b180771f7ef5275b1a6d88e630ca3a3d6f0
Author: Patrick McHardy <kaber@trash.net>
Date:   Fri Feb 19 08:03:28 2010 +0100

    netfilter: restore POST_ROUTING hook in NF_HOOK_COND
    
    Commit 2249065 ("netfilter: get rid of the grossness in netfilter.h")
    inverted the logic for conditional hook invocation, breaking the
    POST_ROUTING hook invoked by ip_output().
    
    Correct the logic and remove an unnecessary initialization.
    
    Reported-by: Stephen Hemminger <shemminger@vyatta.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 7007945..89341c3 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -212,8 +212,9 @@ NF_HOOK_COND(uint8_t pf, unsigned int hook, struct sk_buff *skb,
 	     struct net_device *in, struct net_device *out,
 	     int (*okfn)(struct sk_buff *), bool cond)
 {
-	int ret = 1;
-	if (cond ||
+	int ret;
+
+	if (!cond ||
 	    (ret = nf_hook_thresh(pf, hook, skb, in, out, okfn, INT_MIN) == 1))
 		ret = okfn(skb);
 	return ret;

netfilter 02/03: ctnetlink: fix creation of conntrack with helpers

[Patrick McHardy]

commit a88e22adf5aad79b6e2ddd1bf0109c2ba8b46b0e
Author: Pablo Neira Ayuso <pablo@netfilter.org>
Date:   Fri Feb 19 14:24:39 2010 +0100

    netfilter: ctnetlink: fix creation of conntrack with helpers
    
    This patch fixes a bug that triggers an assertion if you create
    a conntrack entry with a helper and netfilter debugging is enabled.
    Basically, we hit the assertion because the confirmation flag is
    set before the conntrack extensions are added. To fix this, we
    move the extension addition before the aforementioned flag is
    set.
    
    This patch also removes the possibility of setting a helper for
    existing conntracks. This operation would also trigger the
    assertion since we are not allowed to add new extensions for
    existing conntracks. We know noone that could benefit from
    this operation sanely.
    
    Thanks to Eric Dumazet for initial posting a preliminary patch
    to address this issue.
    
    Reported-by: David Ramblewski <David.Ramblewski@atosorigin.com>
    Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
    Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 8b05f36..2b2af63 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1077,9 +1077,8 @@ ctnetlink_change_helper(struct nf_conn *ct, const struct nlattr * const cda[])
 		/* need to zero data of old helper */
 		memset(&help->help, 0, sizeof(help->help));
 	} else {
-		help = nf_ct_helper_ext_add(ct, GFP_ATOMIC);
-		if (help == NULL)
-			return -ENOMEM;
+		/* we cannot set a helper for an existing conntrack */
+		return -EOPNOTSUPP;
 	}
 
 	rcu_assign_pointer(help->helper, helper);
@@ -1263,7 +1262,6 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 	ct->timeout.expires = ntohl(nla_get_be32(cda[CTA_TIMEOUT]));
 
 	ct->timeout.expires = jiffies + ct->timeout.expires * HZ;
-	ct->status |= IPS_CONFIRMED;
 
 	rcu_read_lock();
  	if (cda[CTA_HELP]) {
@@ -1314,14 +1312,19 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 			goto err2;
 	}
 
-	if (cda[CTA_STATUS]) {
-		err = ctnetlink_change_status(ct, cda);
+	if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
+		err = ctnetlink_change_nat(ct, cda);
 		if (err < 0)
 			goto err2;
 	}
 
-	if (cda[CTA_NAT_SRC] || cda[CTA_NAT_DST]) {
-		err = ctnetlink_change_nat(ct, cda);
+	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
+	nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
+	/* we must add conntrack extensions before confirmation. */
+	ct->status |= IPS_CONFIRMED;
+
+	if (cda[CTA_STATUS]) {
+		err = ctnetlink_change_status(ct, cda);
 		if (err < 0)
 			goto err2;
 	}
@@ -1340,9 +1343,6 @@ ctnetlink_create_conntrack(struct net *net, u16 zone,
 			goto err2;
 	}
 
-	nf_ct_acct_ext_add(ct, GFP_ATOMIC);
-	nf_ct_ecache_ext_add(ct, 0, 0, GFP_ATOMIC);
-
 #if defined(CONFIG_NF_CONNTRACK_MARK)
 	if (cda[CTA_MARK])
 		ct->mark = ntohl(nla_get_be32(cda[CTA_MARK]));

netfilter 03/03: nf_queue: fix NF_STOLEN skb leak

[Patrick McHardy]

commit 64507fdbc29c3a622180378210ecea8659b14e40
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Fri Feb 19 15:28:38 2010 +0100

    netfilter: nf_queue: fix NF_STOLEN skb leak
    
    commit 3bc38712e3a6e059 (handle NF_STOP and unknown verdicts in
    nf_reinject) was a partial fix to packet leaks.
    
    If user asks NF_STOLEN status, we must free the skb as well.
    
    Reported-by: Afi Gjermund <afigjermund@gmail.com>
    Signed-off-by: Eric DUmazet <eric.dumazet@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 3a6fd77..ba095fd 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -265,7 +265,6 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 		local_bh_disable();
 		entry->okfn(skb);
 		local_bh_enable();
-	case NF_STOLEN:
 		break;
 	case NF_QUEUE:
 		if (!__nf_queue(skb, elem, entry->pf, entry->hook,
@@ -273,6 +272,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned int verdict)
 				verdict >> NF_VERDICT_BITS))
 			goto next_hook;
 		break;
+	case NF_STOLEN:
 	default:
 		kfree_skb(skb);
 	}

Re: netfilter 00/03: netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Fri, 19 Feb 2010 18:02:06 +0100 (MET)

> git://git.kernel.org/pub/scm/linux/kernel/git/kaber/nf-next-2.6.git master

Pulled, thanks patrick.

Re: netfilter 03/03: nf_queue: fix NF_STOLEN skb leak

[Ondřej Slanina]

Hi,
I know that this patch were applied a long time ago but unfortunately I 
worked all the time on old 2.6.32 kernel. I am confused, because 
documentation said that returning NF_STOLEN means that netfilter release 
ownership of this skb and my hook/queue function is responsible for 
deallocating of this skb. But from now, It's true only for function 
registered by nf_register_hooks (for example defrag function still uses 
NF_STOLEN verdict when performing packed defragmentation). For function 
registered by nf_register_queue_handler() it's not true, because NF_STOLEN 
wil free skb! It's the same as NF_DROP. I have no chance to get ownership of 
skb after call to nf_reinject. Or I am missed something ?
Best Regards,
Ondrej Slanina


-----Původní zpráva----- 
From: Patrick McHardy
Sent: Friday, February 19, 2010 6:02 PM
To: davem@davemloft.net
Cc: netdev@vger.kernel.org ; Patrick McHardy ; 
netfilter-devel@vger.kernel.org
Subject: netfilter 03/03: nf_queue: fix NF_STOLEN skb leak

commit 64507fdbc29c3a622180378210ecea8659b14e40
Author: Eric Dumazet <eric.dumazet@gmail.com>
Date:   Fri Feb 19 15:28:38 2010 +0100

    netfilter: nf_queue: fix NF_STOLEN skb leak

    commit 3bc38712e3a6e059 (handle NF_STOP and unknown verdicts in
    nf_reinject) was a partial fix to packet leaks.

    If user asks NF_STOLEN status, we must free the skb as well.

    Reported-by: Afi Gjermund <afigjermund@gmail.com>
    Signed-off-by: Eric DUmazet <eric.dumazet@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_queue.c b/net/netfilter/nf_queue.c
index 3a6fd77..ba095fd 100644
--- a/net/netfilter/nf_queue.c
+++ b/net/netfilter/nf_queue.c
@@ -265,7 +265,6 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned 
int verdict)
  local_bh_disable();
  entry->okfn(skb);
  local_bh_enable();
- case NF_STOLEN:
  break;
  case NF_QUEUE:
  if (!__nf_queue(skb, elem, entry->pf, entry->hook,
@@ -273,6 +272,7 @@ void nf_reinject(struct nf_queue_entry *entry, unsigned 
int verdict)
  verdict >> NF_VERDICT_BITS))
  goto next_hook;
  break;
+ case NF_STOLEN:
  default:
  kfree_skb(skb);
  }
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" 
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html