linux-kernel - Re: [Stable-review] [104/145] netfilter: xt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4B9BA3C3.50403@canonical.com>
Date:	Sat, 13 Mar 2010 07:40:03 -0700
From:	Tim Gardner <tim.gardner@...onical.com>
To:	Willy Tarreau <w@....eu>
CC:	Greg KH <gregkh@...e.de>, linux-kernel@...r.kernel.org,
	stable@...nel.org, Patrick McHardy <kaber@...sh.net>,
	akpm@...ux-foundation.org, torvalds@...ux-foundation.org,
	stable-review@...nel.org, alan@...rguk.ukuu.org.uk
Subject: Re: [Stable-review] [104/145] netfilter: xt_recent: fix false match

On 03/12/2010 11:24 PM, Willy Tarreau wrote:
> On Fri, Mar 12, 2010 at 04:27:17PM -0800, Greg KH wrote:
>> 2.6.32-stable review patch.  If anyone has any objections, please let me know.
>>
>> ----------------
>> From: Tim Gardner<tim.gardner@...onical.com>
>>
>> commit 8ccb92ad41cb311e52ad1b1fe77992c7f47a3b63 upstream.
>>
>> A rule with a zero hit_count will always match.
>>
>> Signed-off-by: Tim Gardner<tim.gardner@...onical.com>
>> Signed-off-by: Patrick McHardy<kaber@...sh.net>
>> Signed-off-by: Greg Kroah-Hartman<gregkh@...e.de>
>>
>> ---
>>   net/netfilter/xt_recent.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- a/net/netfilter/xt_recent.c
>> +++ b/net/netfilter/xt_recent.c
>> @@ -260,7 +260,7 @@ recent_mt(const struct sk_buff *skb, con
>>   		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;
>>   			}
>
> I don't know if this has any undesired side effect or not, but the
> logic is changed now since "hits" will not be increased anymore when
> info->hit_count is zero. And the code does not make it obvious to me
> what the intended purpose was.
>
> For this reason I always find it dangerous to change variables in
> if() conditions because it's where we change operations the most
> frequently when fixing bugs.
>
> Willy
>

Willy - I agree with you that changing variables in an if() clause can 
be dangerous. I did consider the possibility for side effects in this 
case, but decided to go with the simplest patch since 'hits' is local to 
the scope of the the surrounding else if() clause and is used in no 
other place.

rtg
-- 
Tim Gardner tim.gardner@...onical.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/