[PATCH 00/04]: Netfilter fixes

[PATCH 00/04]: Netfilter fixes

[Patrick McHardy]

Hi Dave,

following are four netfilter fixes for 2.6.27, containing:

- fix for a accidentally introduced static variable in the SIP helper
  that was intended to be local from Alexey

- a GRE helper keymap locking and list iteration fix from Alexey

- a patch to make the IRC helpers' port parsing more robust against the
  issues pointed out by Alexey

Please apply, thanks.


 net/netfilter/nf_conntrack_irc.c       |   10 ++++++++++
 net/netfilter/nf_conntrack_proto_gre.c |   14 +++++++++-----
 net/netfilter/nf_conntrack_sip.c       |    6 ++++--
 3 files changed, 23 insertions(+), 7 deletions(-)

Alexey Dobriyan (3):
      netfilter: nf_conntrack_sip: de-static helper pointers
      netfilter: nf_conntrack_gre: more locking around keymap list
      netfilter: nf_conntrack_gre: nf_ct_gre_keymap_flush() fixlet

Patrick McHardy (1):
      netfilter: nf_conntrack_irc: make sure string is terminated before calling simple_strtoul

[PATCH 01/04]: netfilter: nf_conntrack_sip: de-static helper pointers

[Patrick McHardy]

commit db38a4d83523e816a26a33eef5f7e98262699c90
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Thu Sep 4 14:22:05 2008 +0200

    netfilter: nf_conntrack_sip: de-static helper pointers
    
    Helper's ->help hook can run concurrently with itself, so iterating over
    SIP helpers with static pointer won't work reliably.
    
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_sip.c b/net/netfilter/nf_conntrack_sip.c
index 2f9bbc0..1fa306b 100644
--- a/net/netfilter/nf_conntrack_sip.c
+++ b/net/netfilter/nf_conntrack_sip.c
@@ -1193,7 +1193,6 @@ static const struct sip_handler sip_handlers[] = {
 static int process_sip_response(struct sk_buff *skb,
 				const char **dptr, unsigned int *datalen)
 {
-	static const struct sip_handler *handler;
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 	unsigned int matchoff, matchlen;
@@ -1214,6 +1213,8 @@ static int process_sip_response(struct sk_buff *skb,
 	dataoff = matchoff + matchlen + 1;
 
 	for (i = 0; i < ARRAY_SIZE(sip_handlers); i++) {
+		const struct sip_handler *handler;
+
 		handler = &sip_handlers[i];
 		if (handler->response == NULL)
 			continue;
@@ -1228,13 +1229,14 @@ static int process_sip_response(struct sk_buff *skb,
 static int process_sip_request(struct sk_buff *skb,
 			       const char **dptr, unsigned int *datalen)
 {
-	static const struct sip_handler *handler;
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct = nf_ct_get(skb, &ctinfo);
 	unsigned int matchoff, matchlen;
 	unsigned int cseq, i;
 
 	for (i = 0; i < ARRAY_SIZE(sip_handlers); i++) {
+		const struct sip_handler *handler;
+
 		handler = &sip_handlers[i];
 		if (handler->request == NULL)
 			continue;

Re: [PATCH 01/04]: netfilter: nf_conntrack_sip: de-static helper pointers

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu,  4 Sep 2008 16:15:54 +0200 (MEST)

>     netfilter: nf_conntrack_sip: de-static helper pointers
>     
>     Helper's ->help hook can run concurrently with itself, so iterating over
>     SIP helpers with static pointer won't work reliably.
>     
>     Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[PATCH 02/04]: netfilter: nf_conntrack_gre: more locking around keymap list

[Patrick McHardy]

commit f36591226052551244397113ddccb7677c9f3dbf
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Thu Sep 4 14:22:06 2008 +0200

    netfilter: nf_conntrack_gre: more locking around keymap list
    
    gre_keymap_list should be protected in all places.
    (unless I'm misreading something)
    
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index 654a4f7..b308bb4 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -97,10 +97,14 @@ int nf_ct_gre_keymap_add(struct nf_conn *ct, enum ip_conntrack_dir dir,
 	kmp = &help->help.ct_pptp_info.keymap[dir];
 	if (*kmp) {
 		/* check whether it's a retransmission */
+		read_lock_bh(&nf_ct_gre_lock);
 		list_for_each_entry(km, &gre_keymap_list, list) {
-			if (gre_key_cmpfn(km, t) && km == *kmp)
+			if (gre_key_cmpfn(km, t) && km == *kmp) {
+				read_unlock_bh(&nf_ct_gre_lock);
 				return 0;
+			}
 		}
+		read_unlock_bh(&nf_ct_gre_lock);
 		pr_debug("trying to override keymap_%s for ct %p\n",
 			 dir == IP_CT_DIR_REPLY ? "reply" : "orig", ct);
 		return -EEXIST;

[PATCH 03/04]: netfilter: nf_conntrack_gre: nf_ct_gre_keymap_flush() fixlet

[Patrick McHardy]

commit 73826640c5b48cd471ceca2d904e34b96ba0ecf8
Author: Alexey Dobriyan <adobriyan@gmail.com>
Date:   Thu Sep 4 14:22:06 2008 +0200

    netfilter: nf_conntrack_gre: nf_ct_gre_keymap_flush() fixlet
    
    It does "kfree(list_head)" which looks wrong because entity that was
    allocated is definitely not list_head.
    
    However, this all works because list_head is first item in
    struct nf_ct_gre_keymap.
    
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_proto_gre.c b/net/netfilter/nf_conntrack_proto_gre.c
index b308bb4..9bd0396 100644
--- a/net/netfilter/nf_conntrack_proto_gre.c
+++ b/net/netfilter/nf_conntrack_proto_gre.c
@@ -45,12 +45,12 @@ static LIST_HEAD(gre_keymap_list);
 
 void nf_ct_gre_keymap_flush(void)
 {
-	struct list_head *pos, *n;
+	struct nf_ct_gre_keymap *km, *tmp;
 
 	write_lock_bh(&nf_ct_gre_lock);
-	list_for_each_safe(pos, n, &gre_keymap_list) {
-		list_del(pos);
-		kfree(pos);
+	list_for_each_entry_safe(km, tmp, &gre_keymap_list, list) {
+		list_del(&km->list);
+		kfree(km);
 	}
 	write_unlock_bh(&nf_ct_gre_lock);
 }

Re: [PATCH 03/04]: netfilter: nf_conntrack_gre: nf_ct_gre_keymap_flush() fixlet

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu,  4 Sep 2008 16:15:57 +0200 (MEST)

>     netfilter: nf_conntrack_gre: nf_ct_gre_keymap_flush() fixlet
>     
>     It does "kfree(list_head)" which looks wrong because entity that was
>     allocated is definitely not list_head.
>     
>     However, this all works because list_head is first item in
>     struct nf_ct_gre_keymap.
>     
>     Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Applied.

[PATCH 04/04]: netfilter: nf_conntrack_irc: make sure string is terminated before calling simple_strtoul

[Patrick McHardy]

commit c834a27db90c3b871709c2f3b5d02d5ba62c6c48
Author: Patrick McHardy <kaber@trash.net>
Date:   Thu Sep 4 14:22:06 2008 +0200

    netfilter: nf_conntrack_irc: make sure string is terminated before calling simple_strtoul
    
    Alexey Dobriyan points out:
    
    1. simple_strtoul() silently accepts all characters for given base even
       if result won't fit into unsigned long. This is amazing stupidity in
       itself, but
    
    2. nf_conntrack_irc helper use simple_strtoul() for DCC request parsing.
       Data first copied into 64KB buffer, so theoretically nothing prevents
       reading past the end of it, since data comes from network given 1).
    
    This is not actually a problem currently since we're guaranteed to have
    a 0 byte in skb_shared_info or in the buffer the data is copied to, but
    to make this more robust, make sure the string is actually terminated.
    
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/nf_conntrack_irc.c b/net/netfilter/nf_conntrack_irc.c
index 1b1226d..20633fd 100644
--- a/net/netfilter/nf_conntrack_irc.c
+++ b/net/netfilter/nf_conntrack_irc.c
@@ -68,11 +68,21 @@ static const char *const dccprotos[] = {
 static int parse_dcc(char *data, const char *data_end, u_int32_t *ip,
 		     u_int16_t *port, char **ad_beg_p, char **ad_end_p)
 {
+	char *tmp;
+
 	/* at least 12: "AAAAAAAA P\1\n" */
 	while (*data++ != ' ')
 		if (data > data_end - 12)
 			return -1;
 
+	/* Make sure we have a newline character within the packet boundaries
+	 * because simple_strtoul parses until the first invalid character. */
+	for (tmp = data; tmp <= data_end; tmp++)
+		if (*tmp == '\n')
+			break;
+	if (tmp > data_end || *tmp != '\n')
+		return -1;
+
 	*ad_beg_p = data;
 	*ip = simple_strtoul(data, &data, 10);
 

Re: [PATCH 04/04]: netfilter: nf_conntrack_irc: make sure string is terminated before calling simple_strtoul

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu,  4 Sep 2008 16:15:58 +0200 (MEST)

>     netfilter: nf_conntrack_irc: make sure string is terminated before calling simple_strtoul
>     
>     Alexey Dobriyan points out:
>     
>     1. simple_strtoul() silently accepts all characters for given base even
>        if result won't fit into unsigned long. This is amazing stupidity in
>        itself, but
>     
>     2. nf_conntrack_irc helper use simple_strtoul() for DCC request parsing.
>        Data first copied into 64KB buffer, so theoretically nothing prevents
>        reading past the end of it, since data comes from network given 1).
>     
>     This is not actually a problem currently since we're guaranteed to have
>     a 0 byte in skb_shared_info or in the buffer the data is copied to, but
>     to make this more robust, make sure the string is actually terminated.
>     
>     Signed-off-by: Patrick McHardy <kaber@trash.net>

Also applied, thanks Patrick.

Re: [PATCH 00/04]: Netfilter fixes

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Thu,  4 Sep 2008 16:15:53 +0200 (MEST)

> following are four netfilter fixes for 2.6.27, containing:
> 
> - fix for a accidentally introduced static variable in the SIP helper
>   that was intended to be local from Alexey
> 
> - a GRE helper keymap locking and list iteration fix from Alexey
> 
> - a patch to make the IRC helpers' port parsing more robust against the
>   issues pointed out by Alexey

I applied all of this.

Please limit the scope of anything you send in the future outside of
the merge window.  The "kfree the list head instead of the object itself"
doesn't fix anything, the code worked in it's current form.

These are mostly just cleanups due to code inspection rather than
fixes for bugs that users are actually hitting and reporting.  The
latter (as well as cures for fully exploitable DoS things, obviously)
is the only thing I want to be seeing outside of the merge window.

Thanks.

Re: [PATCH 00/04]: Netfilter fixes

[Patrick McHardy]

David Miller wrote:
> From: Patrick McHardy <kaber@trash.net>
> Date: Thu,  4 Sep 2008 16:15:53 +0200 (MEST)
> 
>> following are four netfilter fixes for 2.6.27, containing:
>>
>> - fix for a accidentally introduced static variable in the SIP helper
>>   that was intended to be local from Alexey
>>
>> - a GRE helper keymap locking and list iteration fix from Alexey
>>
>> - a patch to make the IRC helpers' port parsing more robust against the
>>   issues pointed out by Alexey
> 
> I applied all of this.
> 
> Please limit the scope of anything you send in the future outside of
> the merge window.  The "kfree the list head instead of the object itself"
> doesn't fix anything, the code worked in it's current form.

Indeed, sorry. I was in catch-up mode and apparently didn't think
very much :)