diff mbox

: netfilter: ftp/irc/sane/sip/tftp: Fix the kernel panic when load these modules with duplicated ports

Message ID BAY403-EAS55DEFB461D662C9DA7662795420@phx.gbl
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Feng Gao Sept. 25, 2015, 10:02 a.m. UTC 
Reset the helper->me to NULL when it fail to register helper.
Then the fini function of these modules could avoid unregister
wrong helper.

Signed-off-by: Feng Gao <gfree.wind@gmail.com>
---
 net/netfilter/nf_conntrack_ftp.c  |    1 +
 net/netfilter/nf_conntrack_irc.c  |    7 +++++--
 net/netfilter/nf_conntrack_sane.c |    4 ++++
 net/netfilter/nf_conntrack_sip.c  |    1 +
 net/netfilter/nf_conntrack_tftp.c |    7 +++++--
 5 files changed, 16 insertions(+), 4 deletions(-)

 					tftp[i][j].tuple.src.l3num,
ports[i]);
--
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

Comments

Pablo Neira Ayuso Sept. 25, 2015, 11:20 a.m. UTC | #1 
On Fri, Sep 25, 2015 at 07:12:24PM +0800, Feng Gao wrote:
> Hi Pablo,
> 
> When execute " insmod nf_conntrack_ftp.ko ports=76,76,76", the kernel
> crashes immediately.
> Because the old codes try to unregister the helper which is not registered
> successfully.
> This bug has existed for a long time since the nf_conntrack_helper_register
> checked the duplicated entry.
> 
> I don't know if I should add the reproduce process in the commit comment.

That's good idea to insist on the fact that this bug is crashing the
kernel, and that is not good.
--
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
Pablo Neira Ayuso Oct. 21, 2015, 8:52 a.m. UTC | #2 
On Wed, Oct 21, 2015 at 08:21:12AM +0800, Feng Gao wrote:
> Hi Pablo,
> 
> How about this patch? It will be applied?

Could you investigate if it would be possible to add a
nf_conntrack_helpers_register()?

The idea is to have a native function that handles array of helpers.
We have something similar for _hooks.

Moreover, reduce the length of your patch subject to ~80 chars if
possible.

Thanks.
--
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
diff mbox

Patch

diff --git a/net/netfilter/nf_conntrack_ftp.c
b/net/netfilter/nf_conntrack_ftp.c
index b666959..fbc4b82 100644
--- a/net/netfilter/nf_conntrack_ftp.c
+++ b/net/netfilter/nf_conntrack_ftp.c
@@ -630,6 +630,7 @@  static int __init nf_conntrack_ftp_init(void)
 				 ftp[i][j].tuple.src.l3num, ports[i]);
 			ret = nf_conntrack_helper_register(&ftp[i][j]);
 			if (ret) {
+				ftp[i][j].me = NULL;
 				printk(KERN_ERR "nf_ct_ftp: failed to
register"
 				       " helper for pf: %d port: %d\n",
 					ftp[i][j].tuple.src.l3num,
ports[i]);
diff --git a/net/netfilter/nf_conntrack_irc.c
b/net/netfilter/nf_conntrack_irc.c
index 0fd2976..7c99378 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -267,6 +267,7 @@  static int __init nf_conntrack_irc_init(void)
 
 		ret = nf_conntrack_helper_register(&irc[i]);
 		if (ret) {
+			irc[i].me = NULL;
 			printk(KERN_ERR "nf_ct_irc: failed to register
helper "
 			       "for pf: %u port: %u\n",
 			       irc[i].tuple.src.l3num, ports[i]);
@@ -283,8 +284,10 @@  static void nf_conntrack_irc_fini(void)
 {
 	int i;
 
-	for (i = 0; i < ports_c; i++)
-		nf_conntrack_helper_unregister(&irc[i]);
+	for (i = 0; i < ports_c; i++) {
+		if (irc[i].me)
+			nf_conntrack_helper_unregister(&irc[i]);
+	}
 	kfree(irc_buffer);
 }
 
diff --git a/net/netfilter/nf_conntrack_sane.c
b/net/netfilter/nf_conntrack_sane.c
index 4a2134f..f4986df 100644
--- a/net/netfilter/nf_conntrack_sane.c
+++ b/net/netfilter/nf_conntrack_sane.c
@@ -178,6 +178,9 @@  static void nf_conntrack_sane_fini(void)
 
 	for (i = 0; i < ports_c; i++) {
 		for (j = 0; j < 2; j++) {
+			if (!sane[i][j].me)
+				continue;
+
 			pr_debug("nf_ct_sane: unregistering helper for pf:
%d "
 				 "port: %d\n",
 				 sane[i][j].tuple.src.l3num, ports[i]);
@@ -221,6 +224,7 @@  static int __init nf_conntrack_sane_init(void)
 				 sane[i][j].tuple.src.l3num, ports[i]);
 			ret = nf_conntrack_helper_register(&sane[i][j]);
 			if (ret) {
+				sane[i][j].me = NULL;
 				printk(KERN_ERR "nf_ct_sane: failed to "
 				       "register helper for pf: %d port:
%d\n",
 					sane[i][j].tuple.src.l3num,
ports[i]);
diff --git a/net/netfilter/nf_conntrack_sip.c
b/net/netfilter/nf_conntrack_sip.c
index 885b4ab..53cd48a 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1665,6 +1665,7 @@  static int __init nf_conntrack_sip_init(void)
 
 			ret = nf_conntrack_helper_register(&sip[i][j]);
 			if (ret) {
+				sip[i][j].me = NULL;
 				printk(KERN_ERR "nf_ct_sip: failed to
register"
 				       " helper for pf: %u port: %u\n",
 				       sip[i][j].tuple.src.l3num, ports[i]);
diff --git a/net/netfilter/nf_conntrack_tftp.c
b/net/netfilter/nf_conntrack_tftp.c
index e68ab4f..8255b21 100644
--- a/net/netfilter/nf_conntrack_tftp.c
+++ b/net/netfilter/nf_conntrack_tftp.c
@@ -107,8 +107,10 @@  static void nf_conntrack_tftp_fini(void)
 	int i, j;
 
 	for (i = 0; i < ports_c; i++) {
-		for (j = 0; j < 2; j++)
-			nf_conntrack_helper_unregister(&tftp[i][j]);
+		for (j = 0; j < 2; j++) {
+			if (tftp[i][j].me)
+				nf_conntrack_helper_unregister(&tftp[i][j]);
+		}
 	}
 }
 
@@ -138,6 +140,7 @@  static int __init nf_conntrack_tftp_init(void)
 
 			ret = nf_conntrack_helper_register(&tftp[i][j]);
 			if (ret) {
+				tftp[i][j].me = NULL;
 				printk(KERN_ERR "nf_ct_tftp: failed to
register"
 				       " helper for pf: %u port: %u\n",