[NETFILTER 00/03]: Netfilter fixes

[NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

these patches fix a few netfilter bugs: failure to load IPv4 connection tracking
when loading the NAT module, an invalid return code in ctnetlink and a possible
NULL pointer dereference in ipt_recent. I'll pass the NULL pointer fix to
-stable once its upstream.

Please apply, thanks.


 include/net/netfilter/ipv4/nf_conntrack_ipv4.h |    2 ++
 net/ipv4/netfilter/ipt_recent.c                |    7 ++++++-
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    6 ++++++
 net/ipv4/netfilter/nf_nat_standalone.c         |    2 +-
 net/netfilter/nf_conntrack_netlink.c           |   17 +++++++++--------
 5 files changed, 24 insertions(+), 10 deletions(-)

Jesper Juhl (1):
      [NETFILTER]: ipt_recent: avoid a possible NULL pointer deref in recent_seq_open()

Pablo Neira Ayuso (1):
      [NETFILTER]: ctnetlink: return EEXIST instead of EINVAL for existing nat'ed conntracks

Patrick McHardy (1):
      [NETFILTER]: nf_nat: add symbolic dependency on IPv4 conntrack

[NETFILTER 01/03]: ipt_recent: avoid a possible NULL pointer deref in recent_seq_open()

[Patrick McHardy]

[NETFILTER]: ipt_recent: avoid a possible NULL pointer deref in recent_seq_open()

If the call to seq_open() returns != 0 then the code calls 
kfree(st) but then on the very next line proceeds to 
dereference the pointer - not good.

Problem spotted by the Coverity checker.

Proposed patch to deal with it below.

Compile tested only.

Signed-off-by: Jesper Juhl <jesper.juhl@gmail.com>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit ab3b4927a235c95684cb571b90a04cf6ea1ef7f9
tree acdb734e5cd9dfc4d471ccd85c84cb80100ed45d
parent b880c0879b449ace25e8454656fb0646b32634e6
author Jesper Juhl <jesper.juhl@gmail.com> Mon, 06 Aug 2007 14:09:52 +0200
committer Patrick McHardy <kaber@trash.net> Mon, 06 Aug 2007 14:09:52 +0200

 net/ipv4/netfilter/ipt_recent.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/net/ipv4/netfilter/ipt_recent.c b/net/ipv4/netfilter/ipt_recent.c
index 3218043..6d0c0f7 100644
--- a/net/ipv4/netfilter/ipt_recent.c
+++ b/net/ipv4/netfilter/ipt_recent.c
@@ -387,12 +387,17 @@ static int recent_seq_open(struct inode *inode, struct file *file)
 	st = kzalloc(sizeof(*st), GFP_KERNEL);
 	if (st == NULL)
 		return -ENOMEM;
+
 	ret = seq_open(file, &recent_seq_ops);
-	if (ret)
+	if (ret) {
 		kfree(st);
+		goto out;
+	}
+
 	st->table    = pde->data;
 	seq          = file->private_data;
 	seq->private = st;
+out:
 	return ret;
 }
 

[NETFILTER 02/03]: ctnetlink: return EEXIST instead of EINVAL for existing nat'ed conntracks

[Patrick McHardy]

[NETFILTER]: ctnetlink: return EEXIST instead of EINVAL for existing nat'ed conntracks

ctnetlink must return EEXIST for existing nat'ed conntracks instead of
EINVAL. Only return EINVAL if we try to update a conntrack with NAT
handlings (that is not allowed).

Decadence:libnetfilter_conntrack/utils# ./conntrack_create_nat
TEST: create conntrack (0)(Success)
Decadence:libnetfilter_conntrack/utils# ./conntrack_create_nat
TEST: create conntrack (-1)(Invalid argument)

Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
Signed-off-by: Patrick McHardy <kaber@trash.net>

---
commit 58ff363db6a293220756361af531b11acc5a46e1
tree e94a40ef6d9897c0ac86fe0eedbb9c9d59e3d2b0
parent ab3b4927a235c95684cb571b90a04cf6ea1ef7f9
author Pablo Neira Ayuso <pablo@netfilter.org> Mon, 06 Aug 2007 15:26:39 +0200
committer Patrick McHardy <kaber@trash.net> Mon, 06 Aug 2007 15:26:39 +0200

 net/netfilter/nf_conntrack_netlink.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
index 6f89b10..2863e72 100644
--- a/net/netfilter/nf_conntrack_netlink.c
+++ b/net/netfilter/nf_conntrack_netlink.c
@@ -1052,17 +1052,18 @@ ctnetlink_new_conntrack(struct sock *ctnl, struct sk_buff *skb,
 	}
 	/* implicit 'else' */
 
-	/* we only allow nat config for new conntracks */
-	if (cda[CTA_NAT_SRC-1] || cda[CTA_NAT_DST-1]) {
-		err = -EINVAL;
-		goto out_unlock;
-	}
-
 	/* We manipulate the conntrack inside the global conntrack table lock,
 	 * so there's no need to increase the refcount */
 	err = -EEXIST;
-	if (!(nlh->nlmsg_flags & NLM_F_EXCL))
-		err = ctnetlink_change_conntrack(nf_ct_tuplehash_to_ctrack(h), cda);
+	if (!(nlh->nlmsg_flags & NLM_F_EXCL)) {
+		/* we only allow nat config for new conntracks */
+		if (cda[CTA_NAT_SRC-1] || cda[CTA_NAT_DST-1]) {
+			err = -EINVAL;
+			goto out_unlock;
+		}
+		err = ctnetlink_change_conntrack(nf_ct_tuplehash_to_ctrack(h),
+						 cda);
+	}
 
 out_unlock:
 	write_unlock_bh(&nf_conntrack_lock);

[NETFILTER 03/03]: nf_nat: add symbolic dependency on IPv4 conntrack

[Patrick McHardy]

[NETFILTER]: nf_nat: add symbolic dependency on IPv4 conntrack

Loading nf_nat causes the conntrack core to be loaded, but we need IPv4 as
well.

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

---
commit b92420f32053da61c90aca8cfae3f9be80b2b472
tree 74d4e99ae728c3ee61d3de72c6e48f4846ea8502
parent 58ff363db6a293220756361af531b11acc5a46e1
author Patrick McHardy <kaber@trash.net> Mon, 06 Aug 2007 15:26:39 +0200
committer Patrick McHardy <kaber@trash.net> Mon, 06 Aug 2007 15:26:39 +0200

 include/net/netfilter/ipv4/nf_conntrack_ipv4.h |    2 ++
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c |    6 ++++++
 net/ipv4/netfilter/nf_nat_standalone.c         |    2 +-
 3 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/include/net/netfilter/ipv4/nf_conntrack_ipv4.h b/include/net/netfilter/ipv4/nf_conntrack_ipv4.h
index 7a67160..9bf0598 100644
--- a/include/net/netfilter/ipv4/nf_conntrack_ipv4.h
+++ b/include/net/netfilter/ipv4/nf_conntrack_ipv4.h
@@ -21,4 +21,6 @@ extern struct nf_conntrack_l4proto nf_conntrack_l4proto_icmp;
 extern int nf_conntrack_ipv4_compat_init(void);
 extern void nf_conntrack_ipv4_compat_fini(void);
 
+extern void need_ipv4_conntrack(void);
+
 #endif /*_NF_CONNTRACK_IPV4_H*/
diff --git a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
index 64552af..d9b5177 100644
--- a/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
+++ b/net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c
@@ -509,3 +509,9 @@ static void __exit nf_conntrack_l3proto_ipv4_fini(void)
 
 module_init(nf_conntrack_l3proto_ipv4_init);
 module_exit(nf_conntrack_l3proto_ipv4_fini);
+
+void need_ipv4_conntrack(void)
+{
+	return;
+}
+EXPORT_SYMBOL_GPL(need_ipv4_conntrack);
diff --git a/net/ipv4/netfilter/nf_nat_standalone.c b/net/ipv4/netfilter/nf_nat_standalone.c
index 332814d..46cc99d 100644
--- a/net/ipv4/netfilter/nf_nat_standalone.c
+++ b/net/ipv4/netfilter/nf_nat_standalone.c
@@ -328,7 +328,7 @@ static int __init nf_nat_standalone_init(void)
 {
 	int ret = 0;
 
-	need_conntrack();
+	need_ipv4_conntrack();
 
 #ifdef CONFIG_XFRM
 	BUG_ON(ip_nat_decode_session != NULL);

Re: [NETFILTER 00/03]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Mon,  6 Aug 2007 15:29:03 +0200 (MEST)

> these patches fix a few netfilter bugs: failure to load IPv4 connection tracking
> when loading the NAT module, an invalid return code in ctnetlink and a possible
> NULL pointer dereference in ipt_recent. I'll pass the NULL pointer fix to
> -stable once its upstream.
> 
> Please apply, thanks.

Applied, thanks Patrick.

I really wish those dependencies could be worked out in a nicer
way than calling NULL functions in the needed module.

Re: [NETFILTER 00/03]: Netfilter fixes

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Mon,  6 Aug 2007 15:29:03 +0200 (MEST)
>
>   
>> these patches fix a few netfilter bugs: failure to load IPv4 connection tracking
>> when loading the NAT module, an invalid return code in ctnetlink and a possible
>> NULL pointer dereference in ipt_recent. I'll pass the NULL pointer fix to
>> -stable once its upstream.
>>
>> Please apply, thanks.
>>     
>
> Applied, thanks Patrick.
>
> I really wish those dependencies could be worked out in a nicer
> way than calling NULL functions in the needed module.
>   

Its not very pretty, I agree. In this case we could have used
indirect dependencies and request_module, but I actually prefer
the symbol dependency because its visible in lsmod, which makes
it easier to figure out what needs to be unloaded first to
remove a module.