* [PATCH] netfilter: fix IPv6 NTP checksum calculation
@ 2013-01-02 15:24 Ulrich Weber
2013-01-04 9:20 ` Patrick McHardy
0 siblings, 1 reply; 8+ messages in thread
From: Ulrich Weber @ 2013-01-02 15:24 UTC (permalink / raw)
To: netfilter-devel; +Cc: kaber
csum16_add() has a broken carry detection, should be:
sum += sum < (__force u16)b;
Instead of fixing csum16_add, remove the custom checksum
functions and use the generic csum_add/csum_sub ones.
Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
---
net/ipv6/netfilter/ip6t_NPT.c | 33 +++++++--------------------------
1 file changed, 7 insertions(+), 26 deletions(-)
diff --git a/net/ipv6/netfilter/ip6t_NPT.c b/net/ipv6/netfilter/ip6t_NPT.c
index e948691..7302b0b 100644
--- a/net/ipv6/netfilter/ip6t_NPT.c
+++ b/net/ipv6/netfilter/ip6t_NPT.c
@@ -14,42 +14,23 @@
#include <linux/netfilter_ipv6/ip6t_NPT.h>
#include <linux/netfilter/x_tables.h>
-static __sum16 csum16_complement(__sum16 a)
-{
- return (__force __sum16)(0xffff - (__force u16)a);
-}
-
-static __sum16 csum16_add(__sum16 a, __sum16 b)
-{
- u16 sum;
-
- sum = (__force u16)a + (__force u16)b;
- sum += (__force u16)a < (__force u16)b;
- return (__force __sum16)sum;
-}
-
-static __sum16 csum16_sub(__sum16 a, __sum16 b)
-{
- return csum16_add(a, csum16_complement(b));
-}
-
static int ip6t_npt_checkentry(const struct xt_tgchk_param *par)
{
struct ip6t_npt_tginfo *npt = par->targinfo;
- __sum16 src_sum = 0, dst_sum = 0;
+ __wsum src_sum = 0, dst_sum = 0;
unsigned int i;
if (npt->src_pfx_len > 64 || npt->dst_pfx_len > 64)
return -EINVAL;
for (i = 0; i < ARRAY_SIZE(npt->src_pfx.in6.s6_addr16); i++) {
- src_sum = csum16_add(src_sum,
- (__force __sum16)npt->src_pfx.in6.s6_addr16[i]);
- dst_sum = csum16_add(dst_sum,
- (__force __sum16)npt->dst_pfx.in6.s6_addr16[i]);
+ src_sum = csum_add(src_sum,
+ (__force __wsum)npt->src_pfx.in6.s6_addr16[i]);
+ dst_sum = csum_add(dst_sum,
+ (__force __wsum)npt->dst_pfx.in6.s6_addr16[i]);
}
- npt->adjustment = csum16_sub(src_sum, dst_sum);
+ npt->adjustment = (__force __sum16) csum_sub(src_sum, dst_sum);
return 0;
}
@@ -85,7 +66,7 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
return false;
}
- sum = csum16_add((__force __sum16)addr->s6_addr16[idx],
+ sum = (__force __sum16) csum_add((__force __wsum)addr->s6_addr16[idx],
npt->adjustment);
if (sum == CSUM_MANGLED_0)
sum = 0;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
2013-01-02 15:24 [PATCH] netfilter: fix IPv6 NTP checksum calculation Ulrich Weber
@ 2013-01-04 9:20 ` Patrick McHardy
2013-01-04 19:22 ` Pablo Neira Ayuso
0 siblings, 1 reply; 8+ messages in thread
From: Patrick McHardy @ 2013-01-04 9:20 UTC (permalink / raw)
To: Ulrich Weber; +Cc: netfilter-devel
On Wed, Jan 02, 2013 at 04:24:40PM +0100, Ulrich Weber wrote:
> csum16_add() has a broken carry detection, should be:
> sum += sum < (__force u16)b;
>
> Instead of fixing csum16_add, remove the custom checksum
> functions and use the generic csum_add/csum_sub ones.
Looks good to me.
Acked-by: Patrick McHardy <kaber@trash.net>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
2013-01-04 9:20 ` Patrick McHardy
@ 2013-01-04 19:22 ` Pablo Neira Ayuso
0 siblings, 0 replies; 8+ messages in thread
From: Pablo Neira Ayuso @ 2013-01-04 19:22 UTC (permalink / raw)
To: Patrick McHardy; +Cc: Ulrich Weber, netfilter-devel
On Fri, Jan 04, 2013 at 10:20:43AM +0100, Patrick McHardy wrote:
> On Wed, Jan 02, 2013 at 04:24:40PM +0100, Ulrich Weber wrote:
> > csum16_add() has a broken carry detection, should be:
> > sum += sum < (__force u16)b;
> >
> > Instead of fixing csum16_add, remove the custom checksum
> > functions and use the generic csum_add/csum_sub ones.
>
> Looks good to me.
>
> Acked-by: Patrick McHardy <kaber@trash.net>
Applied, thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
@ 2013-01-24 17:09 Jean-Michel DILLY
2013-01-24 22:49 ` Florian Westphal
0 siblings, 1 reply; 8+ messages in thread
From: Jean-Michel DILLY @ 2013-01-24 17:09 UTC (permalink / raw)
To: netfilter-devel
Hi all,
This patch doesn't work as expected. I tried with few IP and each time 0x0100 were missing. csum_add seems buggy too.
I have reimplemented csum16s functions with the Ulrich's fix. It seems to work now.
I'm a noob, so I guess someone will propose a better patch for this.
diff --git a/ip6t_NPT.c b/linux-3.8-rc4/net/ipv6/netfilter/ip6t_NPT.c
index 787748b..29ef720 100644
--- a/ip6t_NPT.c
+++ b/linux-3.8-rc4/net/ipv6/netfilter/ip6t_NPT.c
@@ -14,24 +14,44 @@
#include <linux/netfilter_ipv6/ip6t_NPT.h>
#include <linux/netfilter/x_tables.h>
+static __sum16 csum16_add(__sum16 a, __sum16 b)
+{
+ __u16 result;
+
+ result = (__force __u16) a + (__force __u16) b;
+ result += result < (__force u16)b;
+
+
+ return (__force __sum16) result;
+}
+
+static __sum16 csum16_complement(__sum16 a)
+{
+ return (__force __sum16)(0xffff - (__force u16)a);
+}
+
+static __sum16 csum16_sub(__sum16 a, __sum16 b)
+{
+ return csum16_add(a, csum16_complement(b));
+}
static int ip6t_npt_checkentry(const struct xt_tgchk_param *par)
{
struct ip6t_npt_tginfo *npt = par->targinfo;
- __wsum src_sum = 0, dst_sum = 0;
+ __sum16 src_sum = 0, dst_sum = 0;
unsigned int i;
if (npt->src_pfx_len > 64 || npt->dst_pfx_len > 64)
return -EINVAL;
for (i = 0; i < ARRAY_SIZE(npt->src_pfx.in6.s6_addr16); i++) {
- src_sum = csum_add(src_sum,
- (__force __wsum)npt->src_pfx.in6.s6_addr16[i]);
- dst_sum = csum_add(dst_sum,
- (__force __wsum)npt->dst_pfx.in6.s6_addr16[i]);
+ src_sum = csum16_add(src_sum,
+ (__force __sum16)npt->src_pfx.in6.s6_addr16[i]);
+ dst_sum = csum16_add(dst_sum,
+ (__force __sum16)npt->dst_pfx.in6.s6_addr16[i]);
}
- npt->adjustment = (__force __sum16) csum_sub(src_sum, dst_sum);
+ npt->adjustment = csum16_sub(src_sum, dst_sum);
return 0;
}
@@ -67,8 +87,8 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
return false;
}
- sum = (__force __sum16) csum_add((__force __wsum)addr->s6_addr16[idx],
- npt->adjustment);
+ sum = csum16_add((__force __sum16)addr->s6_addr16[idx],
+ npt->adjustment);
if (sum == CSUM_MANGLED_0)
sum = 0;
*(__force __sum16 *)&addr->s6_addr16[idx] = sum;
Regards,
JMichel
On Fri, Jan 04, 2013 at 10:20:43AM +0100, Patrick McHardy wrote:
> > On Wed, Jan 02, 2013 at 04:24:40PM +0100, Ulrich Weber wrote:
> > > csum16_add() has a broken carry detection, should be:
> > > sum += sum < (__force u16)b;
> > >
> > > Instead of fixing csum16_add, remove the custom checksum
> > > functions and use the generic csum_add/csum_sub ones.
> >
> > Looks good to me.
> >
> > Acked-by: Patrick McHardy <kaber@trash.net>
>Applied, thanks!
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
2013-01-24 17:09 Jean-Michel DILLY
@ 2013-01-24 22:49 ` Florian Westphal
2013-01-25 21:09 ` Jean-Michel DILLY
0 siblings, 1 reply; 8+ messages in thread
From: Florian Westphal @ 2013-01-24 22:49 UTC (permalink / raw)
To: Jean-Michel DILLY; +Cc: netfilter-devel
Jean-Michel DILLY <jm@dilly.me> wrote:
> Hi all,
> This patch doesn't work as expected. I tried with few IP and each time 0x0100 were missing. csum_add seems buggy too.
> I have reimplemented csum16s functions with the Ulrich's fix. It seems to work now.
> I'm a noob, so I guess someone will propose a better patch for this.
We can't use csum_add after all, patch looks correct.
Can you re-send with proper Signoff?
[ carry detection breaks on overflow, e.g. a==1 and b=0xffff yields
0 for csum_add (65536 < 1) and 1 for csum16_add (0 < 1) ]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
2013-01-24 22:49 ` Florian Westphal
@ 2013-01-25 21:09 ` Jean-Michel DILLY
2013-01-26 16:29 ` YOSHIFUJI Hideaki
0 siblings, 1 reply; 8+ messages in thread
From: Jean-Michel DILLY @ 2013-01-25 21:09 UTC (permalink / raw)
To: Florian Westphal, netfilter-devel
Hi,
Here is the signed-off patch.
Signed-off-by: Jean-Michel DILLY <jm@dilly.me>
---
diff --git a/ip6t_NPT.c b/linux-3.8-rc4/net/ipv6/netfilter/ip6t_NPT.c
index 787748b..29ef720 100644
--- a/ip6t_NPT.c
+++ b/linux-3.8-rc4/net/ipv6/netfilter/ip6t_NPT.c
@@ -14,24 +14,44 @@
#include <linux/netfilter_ipv6/ip6t_NPT.h>
#include <linux/netfilter/x_tables.h>
+static __sum16 csum16_add(__sum16 a, __sum16 b)
+{
+ __u16 result;
+
+ result = (__force __u16) a + (__force __u16) b;
+ result += result < (__force __u16)b;
+
+
+ return (__force __sum16) result;
+}
+
+static __sum16 csum16_complement(__sum16 a)
+{
+ return (__force __sum16)(0xffff - (__force u16)a);
+}
+
+static __sum16 csum16_sub(__sum16 a, __sum16 b)
+{
+ return csum16_add(a, csum16_complement(b));
+}
static int ip6t_npt_checkentry(const struct xt_tgchk_param *par)
{
struct ip6t_npt_tginfo *npt = par->targinfo;
- __wsum src_sum = 0, dst_sum = 0;
+ __sum16 src_sum = 0, dst_sum = 0;
unsigned int i;
if (npt->src_pfx_len > 64 || npt->dst_pfx_len > 64)
return -EINVAL;
for (i = 0; i < ARRAY_SIZE(npt->src_pfx.in6.s6_addr16); i++) {
- src_sum = csum_add(src_sum,
- (__force __wsum)npt->src_pfx.in6.s6_addr16[i]);
- dst_sum = csum_add(dst_sum,
- (__force __wsum)npt->dst_pfx.in6.s6_addr16[i]);
+ src_sum = csum16_add(src_sum,
+ (__force __sum16)npt->src_pfx.in6.s6_addr16[i]);
+ dst_sum = csum16_add(dst_sum,
+ (__force __sum16)npt->dst_pfx.in6.s6_addr16[i]);
}
- npt->adjustment = (__force __sum16) csum_sub(src_sum, dst_sum);
+ npt->adjustment = csum16_sub(src_sum, dst_sum);
return 0;
}
@@ -67,8 +87,8 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
return false;
}
- sum = (__force __sum16) csum_add((__force __wsum)addr->s6_addr16[idx],
- npt->adjustment);
+ sum = csum16_add((__force __sum16)addr->s6_addr16[idx],
+ npt->adjustment);
if (sum == CSUM_MANGLED_0)
sum = 0;
*(__force __sum16 *)&addr->s6_addr16[idx] = sum;
Le 24 janv. 2013 à 23:49, Florian Westphal a écrit :
> Jean-Michel DILLY <jm@dilly.me> wrote:
>> Hi all,
>> This patch doesn't work as expected. I tried with few IP and each time 0x0100 were missing. csum_add seems buggy too.
>> I have reimplemented csum16s functions with the Ulrich's fix. It seems to work now.
>> I'm a noob, so I guess someone will propose a better patch for this.
>
> We can't use csum_add after all, patch looks correct.
>
> Can you re-send with proper Signoff?
>
> [ carry detection breaks on overflow, e.g. a==1 and b=0xffff yields
> 0 for csum_add (65536 < 1) and 1 for csum16_add (0 < 1) ]
> --
> 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
--
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
2013-01-25 21:09 ` Jean-Michel DILLY
@ 2013-01-26 16:29 ` YOSHIFUJI Hideaki
2013-01-29 14:50 ` Ulrich Weber
0 siblings, 1 reply; 8+ messages in thread
From: YOSHIFUJI Hideaki @ 2013-01-26 16:29 UTC (permalink / raw)
To: Jean-Michel DILLY; +Cc: Florian Westphal, netfilter-devel, YOSHIFUJI Hideaki
Jean-Michel DILLY wrote:
> Hi,
>
> Here is the signed-off patch.
>
> Signed-off-by: Jean-Michel DILLY <jm@dilly.me>
> ---
> diff --git a/ip6t_NPT.c b/linux-3.8-rc4/net/ipv6/netfilter/ip6t_NPT.c
> index 787748b..29ef720 100644
> --- a/ip6t_NPT.c
> +++ b/linux-3.8-rc4/net/ipv6/netfilter/ip6t_NPT.c
> @@ -14,24 +14,44 @@
> #include <linux/netfilter_ipv6/ip6t_NPT.h>
> #include <linux/netfilter/x_tables.h>
>
> +static __sum16 csum16_add(__sum16 a, __sum16 b)
> +{
> + __u16 result;
> +
> + result = (__force __u16) a + (__force __u16) b;
> + result += result < (__force __u16)b;
> +
> +
> + return (__force __sum16) result;
> +}
> +
> +static __sum16 csum16_complement(__sum16 a)
> +{
> + return (__force __sum16)(0xffff - (__force u16)a);
> +}
> +
> +static __sum16 csum16_sub(__sum16 a, __sum16 b)
> +{
> + return csum16_add(a, csum16_complement(b));
> +}
>
> static int ip6t_npt_checkentry(const struct xt_tgchk_param *par)
> {
> struct ip6t_npt_tginfo *npt = par->targinfo;
> - __wsum src_sum = 0, dst_sum = 0;
> + __sum16 src_sum = 0, dst_sum = 0;
> unsigned int i;
>
> if (npt->src_pfx_len > 64 || npt->dst_pfx_len > 64)
> return -EINVAL;
>
> for (i = 0; i < ARRAY_SIZE(npt->src_pfx.in6.s6_addr16); i++) {
> - src_sum = csum_add(src_sum,
> - (__force __wsum)npt->src_pfx.in6.s6_addr16[i]);
> - dst_sum = csum_add(dst_sum,
> - (__force __wsum)npt->dst_pfx.in6.s6_addr16[i]);
> + src_sum = csum16_add(src_sum,
> + (__force __sum16)npt->src_pfx.in6.s6_addr16[i]);
> + dst_sum = csum16_add(dst_sum,
> + (__force __sum16)npt->dst_pfx.in6.s6_addr16[i]);
> }
>
> - npt->adjustment = (__force __sum16) csum_sub(src_sum, dst_sum);
> + npt->adjustment = csum16_sub(src_sum, dst_sum);
> return 0;
> }
>
> @@ -67,8 +87,8 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
> return false;
> }
>
> - sum = (__force __sum16) csum_add((__force __wsum)addr->s6_addr16[idx],
> - npt->adjustment);
> + sum = csum16_add((__force __sum16)addr->s6_addr16[idx],
> + npt->adjustment);
> if (sum == CSUM_MANGLED_0)
> sum = 0;
> *(__force __sum16 *)&addr->s6_addr16[idx] = sum;
>
> Le 24 janv. 2013 à 23:49, Florian Westphal a écrit :
>
>> Jean-Michel DILLY <jm@dilly.me> wrote:
>>> Hi all,
>>> This patch doesn't work as expected. I tried with few IP and each time 0x0100 were missing. csum_add seems buggy too.
>>> I have reimplemented csum16s functions with the Ulrich's fix. It seems to work now.
>>> I'm a noob, so I guess someone will propose a better patch for this.
>>
>> We can't use csum_add after all, patch looks correct.
>>
>> Can you re-send with proper Signoff?
>>
>> [ carry detection breaks on overflow, e.g. a==1 and b=0xffff yields
>> 0 for csum_add (65536 < 1) and 1 for csum16_add (0 < 1) ]
Well... why do you fill npt->adjustment?
I think we can do checksumming in ip6t_npt_map_pfx()
with csum_partial(), csum_block_add()/csum_block_sub() and
csum_fold().
--yoshfuji
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] netfilter: fix IPv6 NTP checksum calculation
2013-01-26 16:29 ` YOSHIFUJI Hideaki
@ 2013-01-29 14:50 ` Ulrich Weber
0 siblings, 0 replies; 8+ messages in thread
From: Ulrich Weber @ 2013-01-29 14:50 UTC (permalink / raw)
To: Jean-Michel DILLY; +Cc: YOSHIFUJI Hideaki, Florian Westphal, netfilter-devel
[-- Attachment #1: Type: text/plain, Size: 256 bytes --]
Hi Jean-Michel,
can you please test again with Yoshifuji's patches and attached patch?
I think csum16_add() is still not proper, we would also need a carry bit if
"result < a". We better use the internal checksum functions if possible...
Cheers
Ulrich
[-- Attachment #2: 0001-netfilter-ip6t_NTP-Use-onces-complement-of-csum_fold.patch --]
[-- Type: text/x-patch, Size: 1412 bytes --]
>From 40e0c6d86514a8dcc80f18fbe8a2945c6ee78f6d Mon Sep 17 00:00:00 2001
From: Ulrich Weber <ulrich.weber@sophos.com>
Date: Tue, 29 Jan 2013 15:24:21 +0100
Subject: [PATCH] netfilter: ip6t_NTP: Use onces complement of csum_fold
we need a 16bit value but not folded
Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
---
net/ipv6/netfilter/ip6t_NPT.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/netfilter/ip6t_NPT.c b/net/ipv6/netfilter/ip6t_NPT.c
index 74e171d..61a9b95 100644
--- a/net/ipv6/netfilter/ip6t_NPT.c
+++ b/net/ipv6/netfilter/ip6t_NPT.c
@@ -35,7 +35,7 @@ static int ip6t_npt_checkentry(const struct xt_tgchk_param *par)
src_sum = csum_partial(&npt->src_pfx.in6, sizeof(npt->src_pfx.in6), 0);
dst_sum = csum_partial(&npt->dst_pfx.in6, sizeof(npt->dst_pfx.in6), 0);
- npt->adjustment = csum_fold(csum_sub(src_sum, dst_sum));
+ npt->adjustment = ~csum_fold(csum_sub(src_sum, dst_sum));
return 0;
}
@@ -71,8 +71,8 @@ static bool ip6t_npt_map_pfx(const struct ip6t_npt_tginfo *npt,
return false;
}
- sum = csum_fold(csum_add(csum_unfold((__force __sum16)addr->s6_addr16[idx]),
- csum_unfold(npt->adjustment)));
+ sum = ~csum_fold(csum_add(csum_unfold((__force __sum16)addr->s6_addr16[idx]),
+ csum_unfold(npt->adjustment)));
if (sum == CSUM_MANGLED_0)
sum = 0;
*(__force __sum16 *)&addr->s6_addr16[idx] = sum;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-01-29 14:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-02 15:24 [PATCH] netfilter: fix IPv6 NTP checksum calculation Ulrich Weber
2013-01-04 9:20 ` Patrick McHardy
2013-01-04 19:22 ` Pablo Neira Ayuso
-- strict thread matches above, loose matches on Subject: below --
2013-01-24 17:09 Jean-Michel DILLY
2013-01-24 22:49 ` Florian Westphal
2013-01-25 21:09 ` Jean-Michel DILLY
2013-01-26 16:29 ` YOSHIFUJI Hideaki
2013-01-29 14:50 ` Ulrich Weber
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).