[PATCH] xt_recent: Fix false hit_count match

[PATCH] xt_recent: Fix false hit_count match

[Tim Gardner]

>From 146111514a8c126268e848e45b7dd967329b072f Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Thu, 18 Feb 2010 20:33:00 -0700
Subject: [PATCH] xt_recent: Fix false match.

A rule with a zero hit_count will always match.

Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
Cc: stable@kernel.org
---
 net/netfilter/xt_recent.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 1bb0d6c..43e83a4 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -260,7 +260,7 @@ recent_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 		for (i = 0; i < e->nstamps; i++) {
 			if (info->seconds && time_after(time, e->stamps[i]))
 				continue;
-			if (++hits >= info->hit_count) {
+			if (info->hit_count && ++hits >= info->hit_count) {
 				ret = !ret;
 				break;
 			}
-- 
1.6.2.4

Re: [PATCH] xt_recent: Fix false hit_count match

[Patrick McHardy]

Tim Gardner wrote:
>>From 146111514a8c126268e848e45b7dd967329b072f Mon Sep 17 00:00:00 2001
> From: Tim Gardner <tim.gardner@canonical.com>
> Date: Thu, 18 Feb 2010 20:33:00 -0700
> Subject: [PATCH] xt_recent: Fix false match.
> 
> A rule with a zero hit_count will always match.
> 

Also applied, thanks Tim.

Re: [PATCH] xt_recent: Fix false hit_count match

[Thomas Jarosch]

On Tuesday, 23. February 2010 14:59:46 Patrick McHardy wrote:
> Tim Gardner wrote:
> >>From 146111514a8c126268e848e45b7dd967329b072f Mon Sep 17 00:00:00 2001
> >>
> > From: Tim Gardner <tim.gardner@canonical.com>
> > Date: Thu, 18 Feb 2010 20:33:00 -0700
> > Subject: [PATCH] xt_recent: Fix false match.
> > 
> > A rule with a zero hit_count will always match.
> 
> Also applied, thanks Tim.

I just updated from kernel 2.6.32.9 to kernel 2.6.32.10 which contains
the xt_recent "zero hit_count will always match" fix.

After that xt_recent stopped working for this scenario:

iptables -A INPUT -m recent --rcheck --rdest --name INET_IP -j LOG
echo "+1.2.3.4" >/proc/net/xt_recent/INET_IP

The ip address 1.2.3.4 represents the current ip of my dial up connection.

If I change "--rcheck" to "--update", it works again.
Reverting the patch fixes the issue.

Maybe this is related to the xt_recent
proc interface creating the entry
(with a zero hit count)?

Cheers,
Thomas

Re: [PATCH] xt_recent: Fix false hit_count match

[Patrick McHardy]

Thomas Jarosch wrote:
> On Tuesday, 23. February 2010 14:59:46 Patrick McHardy wrote:
>   
>> Tim Gardner wrote:
>>     
>>> >From 146111514a8c126268e848e45b7dd967329b072f Mon Sep 17 00:00:00 2001
>>>       
>>> From: Tim Gardner <tim.gardner@canonical.com>
>>> Date: Thu, 18 Feb 2010 20:33:00 -0700
>>> Subject: [PATCH] xt_recent: Fix false match.
>>>
>>> A rule with a zero hit_count will always match.
>>>       
>> Also applied, thanks Tim.
>>     
>
> I just updated from kernel 2.6.32.9 to kernel 2.6.32.10 which contains
> the xt_recent "zero hit_count will always match" fix.
>
> After that xt_recent stopped working for this scenario:
>
> iptables -A INPUT -m recent --rcheck --rdest --name INET_IP -j LOG
> echo "+1.2.3.4" >/proc/net/xt_recent/INET_IP
>
> The ip address 1.2.3.4 represents the current ip of my dial up connection.
>
> If I change "--rcheck" to "--update", it works again.
> Reverting the patch fixes the issue.
>
> Maybe this is related to the xt_recent
> proc interface creating the entry
> (with a zero hit count)?
>   

Mhh, looking at that patch again, I think it should actually do:

if (!info->hit_count || ++hits >= info->hit_count)
    ...

since a hit_count of 0 implies that the user just wants to check for the
presence of the entry. Thomas, could you give that a try?

Re: [PATCH] xt_recent: Fix false hit_count match

[Tim Gardner]

On 03/19/2010 09:41 AM, Patrick McHardy wrote:
> Thomas Jarosch wrote:
>> On Tuesday, 23. February 2010 14:59:46 Patrick McHardy wrote:
>>
>>> Tim Gardner wrote:
>>>
>>>> > From 146111514a8c126268e848e45b7dd967329b072f Mon Sep 17 00:00:00 2001
>>>>
>>>> From: Tim Gardner<tim.gardner@canonical.com>
>>>> Date: Thu, 18 Feb 2010 20:33:00 -0700
>>>> Subject: [PATCH] xt_recent: Fix false match.
>>>>
>>>> A rule with a zero hit_count will always match.
>>>>
>>> Also applied, thanks Tim.
>>>
>>
>> I just updated from kernel 2.6.32.9 to kernel 2.6.32.10 which contains
>> the xt_recent "zero hit_count will always match" fix.
>>
>> After that xt_recent stopped working for this scenario:
>>
>> iptables -A INPUT -m recent --rcheck --rdest --name INET_IP -j LOG
>> echo "+1.2.3.4">/proc/net/xt_recent/INET_IP
>>
>> The ip address 1.2.3.4 represents the current ip of my dial up connection.
>>
>> If I change "--rcheck" to "--update", it works again.
>> Reverting the patch fixes the issue.
>>
>> Maybe this is related to the xt_recent
>> proc interface creating the entry
>> (with a zero hit count)?
>>
>
> Mhh, looking at that patch again, I think it should actually do:
>
> if (!info->hit_count || ++hits>= info->hit_count)
>      ...
>
> since a hit_count of 0 implies that the user just wants to check for the
> presence of the entry. Thomas, could you give that a try?
>

I think you're right. Its kind of a subtle exit condition.

rtg
-- 
Tim Gardner timg@tpi.com www.tpi.com
OR 503-601-0234 x102 MT 406-443-5357

Re: [PATCH] xt_recent: Fix false hit_count match

[Thomas Jarosch]

On Friday, 19. March 2010 16:41:49 you wrote:
> > Maybe this is related to the xt_recent
> > proc interface creating the entry
> > (with a zero hit count)?
> 
> Mhh, looking at that patch again, I think it should actually do:
> 
> if (!info->hit_count || ++hits >= info->hit_count)
>     ...
> 
> since a hit_count of 0 implies that the user just wants to check for the
> presence of the entry. Thomas, could you give that a try?

The new code works. Isn't that almost the same as reverting
the original patch? info->hit_count == 0 will match again.

So we could just go back to

"if (++hits >= info->hit_count)"

Or am I missing something?

Clearly your new version is more readable about the intent.

Cheers,
Thomas

Re: [PATCH] xt_recent: Fix false hit_count match

[Patrick McHardy]

Thomas Jarosch wrote:
> On Friday, 19. March 2010 16:41:49 you wrote:
>   
>>> Maybe this is related to the xt_recent
>>> proc interface creating the entry
>>> (with a zero hit count)?
>>>       
>> Mhh, looking at that patch again, I think it should actually do:
>>
>> if (!info->hit_count || ++hits >= info->hit_count)
>>     ...
>>
>> since a hit_count of 0 implies that the user just wants to check for the
>> presence of the entry. Thomas, could you give that a try?
>>     
>
> The new code works. Isn't that almost the same as reverting
> the original patch? info->hit_count == 0 will match again.
>
> So we could just go back to
>
> "if (++hits >= info->hit_count)"
>
> Or am I missing something?
>   

I think you're right. Tim, please remind me, why was the match on zero
hits considered a false positive?

Re: [PATCH] xt_recent: Fix false hit_count match

[Tim Gardner]

On 03/19/2010 10:32 AM, Patrick McHardy wrote:
> Thomas Jarosch wrote:
>> On Friday, 19. March 2010 16:41:49 you wrote:
>>
>>>> Maybe this is related to the xt_recent
>>>> proc interface creating the entry
>>>> (with a zero hit count)?
>>>>
>>> Mhh, looking at that patch again, I think it should actually do:
>>>
>>> if (!info->hit_count || ++hits>= info->hit_count)
>>>      ...
>>>
>>> since a hit_count of 0 implies that the user just wants to check for the
>>> presence of the entry. Thomas, could you give that a try?
>>>
>>
>> The new code works. Isn't that almost the same as reverting
>> the original patch? info->hit_count == 0 will match again.
>>
>> So we could just go back to
>>
>> "if (++hits>= info->hit_count)"
>>
>> Or am I missing something?
>>
>
> I think you're right. Tim, please remind me, why was the match on zero
> hits considered a false positive?
>

Because it looked like it? Maybe its just whining after the fact, but 3 
of us missed that it was also an exit condition. IMHO it was too subtle. 
I like your final patch much better because, as Thomas pointed out, it 
makes it a bit clearer what that clause is doing.

rtg
-- 
Tim Gardner timg@tpi.com www.tpi.com
OR 503-601-0234 x102 MT 406-443-5357

Re: [PATCH] xt_recent: Fix false hit_count match

[Patrick McHardy]

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

Thomas Jarosch wrote:
> On Friday, 19. March 2010 16:41:49 you wrote:
>   
>>> Maybe this is related to the xt_recent
>>> proc interface creating the entry
>>> (with a zero hit count)?
>>>       
>> Mhh, looking at that patch again, I think it should actually do:
>>
>> if (!info->hit_count || ++hits >= info->hit_count)
>>     ...
>>
>> since a hit_count of 0 implies that the user just wants to check for the
>> presence of the entry. Thomas, could you give that a try?
>>     
>
> The new code works. Isn't that almost the same as reverting
> the original patch? info->hit_count == 0 will match again.
>
> So we could just go back to
>
> "if (++hits >= info->hit_count)"
>
> Or am I missing something?
>
> Clearly your new version is more readable about the intent.

Thomas, before I send this upstream with a Tested-by tag in your name,
could you please confirm that this is the change you've actually tested?

Thanks.



[-- Attachment #2: x --]
[-- Type: text/plain, Size: 1462 bytes --]

commit ef1691504c83ba3eb636c0cfd3ed33f7a6d0b4ee
Author: Patrick McHardy <kaber@trash.net>
Date:   Mon Mar 22 18:25:20 2010 +0100

    netfilter: xt_recent: fix regression in rules using a zero hit_count
    
    Commit 8ccb92ad (netfilter: xt_recent: fix false match) fixed supposedly
    false matches in rules using a zero hit_count. As it turns out there is
    nothing false about these matches and people are actually using entries
    with a hit_count of zero to make rules dependant on addresses inserted
    manually through /proc.
    
    Since this slipped past the eyes of three reviewers, instead of
    reverting the commit in question, this patch explicitly checks
    for a hit_count of zero to make the intentions more clear.
    
    Reported-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
    Tested-by: Thomas Jarosch <thomas.jarosch@intra2net.com>
    Cc: stable@kernel.org
    Signed-off-by: Patrick McHardy <kaber@trash.net>

diff --git a/net/netfilter/xt_recent.c b/net/netfilter/xt_recent.c
index 7073dbb..971d172 100644
--- a/net/netfilter/xt_recent.c
+++ b/net/netfilter/xt_recent.c
@@ -267,7 +267,7 @@ recent_mt(const struct sk_buff *skb, const struct xt_match_param *par)
 		for (i = 0; i < e->nstamps; i++) {
 			if (info->seconds && time_after(time, e->stamps[i]))
 				continue;
-			if (info->hit_count && ++hits >= info->hit_count) {
+			if (!info->hit_count || ++hits >= info->hit_count) {
 				ret = !ret;
 				break;
 			}

Re: [PATCH] xt_recent: Fix false hit_count match

[Thomas Jarosch]

On 03/22/2010 06:31 PM, Patrick McHardy wrote:
>> Clearly your new version is more readable about the intent.
> 
> Thomas, before I send this upstream with a Tested-by tag in your name,
> could you please confirm that this is the change you've actually tested?

Yes and I just did another test run with your final patch, works fine.

Cheers,
Thomas