Fwd: xt_recent compat code supposedly broken

Fwd: xt_recent compat code supposedly broken

[Jan Engelhardt]

[Cc: netfilter-devel and people that touched the code v2.6.28..v2.6.29]

>---------- Forwarded message ----------
>Date: Thu, 23 Apr 2009 17:52:12
>From: Roman Hoog Antink
>To: Jan Engelhardt
>
>Linux kernels 2.6.28 and 2.6.29 seem to have troubles, applying iptables
>rules correctly that use the recent match.
>
>See here for a bug description:
>https://bugs.launchpad.net/ubuntu/+source/linux/+bug/365539
>
>The duplicate entries can be created with the new /proc/net/xt_recent/
>files only. Successive "echo IP >/proc/net/xt_recent/test" calls cause
>a double entry of IP. More echo's wont increase the number of duplicates.
>This effect occurred with 2.6.29 only sporadically. After booting the
>kernel the first time, it worked for some hours (jiffies overrun?), then
>it stopped working (without reboot). When investigating the next day in the
>morning, the problem was there again and right this afternoon it vanished.
>
>The duplicate entries occur always together with the ignored recent rules.
>
>The denied removal of entries (echo -IP >/proc/net/xt_recent/test) only
>occurs on Ubuntu Jaunty Beta (linux 2.6.28), where
>CONFIG_NETFILTER_XT_MATCH_RECENT_PROC_COMPAT is not set. And here I was
>able to produce more than 2 duplicate entries by successive echo +IP >..
>executions. The flush command '/' works correctly in any case.
>
>I am sorry to report a sporadic problem, as I painfully know, they are the
>hardest to track down.
>
>---------- Forwarded message ----------
>Date: Fri, 24 Apr 2009 09:19:51
>
>I updated the bug description
>https://bugs.launchpad.net/ubuntu/+source/linux/+bug/365539
>
>It seems, that on kernel 2.6.29, only the COMPAT option is buggy. On kernel
>2.6.28 (used by Jaunty) however, xt_recent.ko has no effect on iptables rules.

Re: xt_recent compat code supposedly broken

[Jan Engelhardt]

On Friday 2009-04-24 10:43, Jan Engelhardt wrote:

>>Linux kernels 2.6.28 and 2.6.29 seem to have troubles, applying iptables
>>rules correctly that use the recent match.

The standard code has been fixed in 2.6.29 by Josef Drexler.

>>It seems, that on kernel 2.6.29, only the COMPAT option is buggy. On kernel
>>2.6.28 (used by Jaunty) however, xt_recent.ko has no effect on iptables rules.

Right. Here is a compile-tested patch. When confirmed by Roman (seems to 
have a source tree handy ;-) , this should be going into -stable too. 
Affected: 2.6.28, 2.6.29, running 2.6.30-rc.
Those casts pretty much looked suspicious enough, heh.

I guess now I can also reply to 325fb5b4's question-in-commit: "Possible 
solutions: (1) initialize the addr variable in recent_mt_proc_write, or 
(2) compare only 4 bytes for IPv4 addresses in recent_entry_lookup". The 
presence of the cast that are being removed with this patch suggest that 
originally, somehow, only X bytes were compared, depending on NFPROTO_* 
that was passed around. Looks like that never came about though, so I 
think we should stick with (1)-style code for xt_recent.

--->8---
parent 415a0779776ba14dd7895e13c02519f76fab0b65 (v2.6.30-rc1-424-g415a077)
commit e7051c8bd856f4197cdb791dfa76ab31ba07824d
Author: Jan Engelhardt <jengelh@medozas.de>
Date:   Fri Apr 24 10:42:12 2009 +0200

netfilter: xt_recent: fix stack overread in compat code

Related-to: commit 325fb5b4d26038cba665dd0d8ee09555321061f0

The compat path suffers from a similar problem. It only uses a __be32
when all of the recent code uses, and expects, an nf_inet_addr
everywhere. As a result, addresses stored by xt_recents were
filled with whatever other stuff was on the stack following the be32.

Reported-by: Roman Hoog Antink <rha@open.ch>
Signed-off-by: Jan Engelhardt <jengelh@medozas.de>
---
 net/netfilter/xt_recent.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 791e030..fcc2d7b 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -474,7 +474,7 @@ static ssize_t recent_old_proc_write(struct file *file,
 	struct recent_table *t = pde->data;
 	struct recent_entry *e;
 	char buf[sizeof("+255.255.255.255")], *c = buf;
-	__be32 addr;
+	struct nf_inet_addr addr = {};
 	int add;
 
 	if (size > sizeof(buf))
@@ -506,14 +506,13 @@ static ssize_t recent_old_proc_write(struct file *file,
 		add = 1;
 		break;
 	}
-	addr = in_aton(c);
+	addr.ip = in_aton(c);
 
 	spin_lock_bh(&recent_lock);
-	e = recent_entry_lookup(t, (const void *)&addr, NFPROTO_IPV4, 0);
+	e = recent_entry_lookup(t, &addr, NFPROTO_IPV4, 0);
 	if (e == NULL) {
 		if (add)
-			recent_entry_init(t, (const void *)&addr,
-					  NFPROTO_IPV4, 0);
+			recent_entry_init(t, &addr, NFPROTO_IPV4, 0);
 	} else {
 		if (add)
 			recent_entry_update(t, e);
-- 
# Created with git-export-patch

Re: xt_recent compat code supposedly broken

[Roman Hoog Antink]

[-- Attachment #1: Type: text/plain, Size: 189 bytes --]

Thank you!

Find attached the slightly adapted patch (nf_inet_addr is a union, not 
struct). I tested it successfully with kernel 2.6.29 and it fixed the 
compat path issue.

Cheers
Roman


[-- Attachment #2: xt_recent-compat-2.6.29.patch --]
[-- Type: text/x-patch, Size: 983 bytes --]

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 791e030..fcc2d7b 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -474,7 +474,7 @@ static ssize_t recent_old_proc_write(struct file *file,
 	struct recent_table *t = pde->data;
 	struct recent_entry *e;
 	char buf[sizeof("+255.255.255.255")], *c = buf;
-	__be32 addr;
+	union nf_inet_addr addr = {};
 	int add;
 
 	if (size > sizeof(buf))
@@ -506,14 +506,13 @@ static ssize_t recent_old_proc_write(struct file *file,
 		add = 1;
 		break;
 	}
-	addr = in_aton(c);
+	addr.ip = in_aton(c);
 
 	spin_lock_bh(&recent_lock);
-	e = recent_entry_lookup(t, (const void *)&addr, NFPROTO_IPV4, 0);
+	e = recent_entry_lookup(t, &addr, NFPROTO_IPV4, 0);
 	if (e == NULL) {
 		if (add)
-			recent_entry_init(t, (const void *)&addr,
-					  NFPROTO_IPV4, 0);
+			recent_entry_init(t, &addr, NFPROTO_IPV4, 0);
 	} else {
 		if (add)
 			recent_entry_update(t, e);
-- # Created with git-export-patch 

Re: xt_recent compat code supposedly broken

[Patrick McHardy]

Roman Hoog Antink wrote:
> Thank you!
> 
> Find attached the slightly adapted patch (nf_inet_addr is a union, not 
> struct). I tested it successfully with kernel 2.6.29 and it fixed the 
> compat path issue.

Applied, thanks everyone.