Patchwork netfilter: fix IPv6 NTP checksum calculation

login
register
mail settings
Submitter Jean-Michel DILLY
Date Jan. 25, 2013, 9:09 p.m.
Message ID <AD70451A-D1CE-4B5A-AB9B-B30FDCD0FB7F@dilly.me>
Download mbox | patch
Permalink /patch/215869/
State Not Applicable
Headers show

Comments

Jean-Michel DILLY - Jan. 25, 2013, 9:09 p.m.
Hi,

Here is the signed-off patch.

Signed-off-by: Jean-Michel DILLY <jm@dilly.me>
---

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
YOSHIFUJI Hideaki / 吉藤英明 - Jan. 26, 2013, 4:29 p.m.
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

Patch

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;