bug : nft_redirect port byteorder issue

bug : nft_redirect port byteorder issue

[leroy christophe]

Hi,

table ip nat {
         chain prerouting {
                  type nat hook prerouting priority 0;
                  tcp dport 222 redirect :22
         }
         chain postrouting {
                  type nat hook postrouting priority 0;
         }
}

With the above rules, data[priv->sreg_proto_min].data[0] has value 
0x160000 instead of 0x16 on powerpc (Big Endian byte order)

Due to this, mr.range[0].min.all gets assigned value 0 instead of 22.

Below patch fixes it, but it is maybe not the proper way to fix it, so I 
let it up to you.

Christophe

diff --git a/net/ipv4/netfilter/nft_redir_ipv4.c 
b/net/ipv4/netfilter/nft_redir_ipv4.c
index 643c596..554bb32 100644
--- a/net/ipv4/netfilter/nft_redir_ipv4.c
+++ b/net/ipv4/netfilter/nft_redir_ipv4.c
@@ -28,9 +28,9 @@ static void nft_redir_ipv4_eval(const struct nft_expr 
*expr,
  	memset(&mr, 0, sizeof(mr));
  	if (priv->sreg_proto_min) {
  		mr.range[0].min.all = (__force __be16)
-					data[priv->sreg_proto_min].data[0];
+					*(__be16*)&data[priv->sreg_proto_min].data[0];
  		mr.range[0].max.all = (__force __be16)
-					data[priv->sreg_proto_max].data[0];
+					*(__be16*)&data[priv->sreg_proto_max].data[0];
  		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
  	}

Re: bug : nft_redirect port byteorder issue

[Arturo Borrero Gonzalez]

On 12 December 2014 at 11:16, leroy christophe <christophe.leroy@c-s.fr> wrote:
> Hi,
>
> table ip nat {
>         chain prerouting {
>                  type nat hook prerouting priority 0;
>                  tcp dport 222 redirect :22
>         }
>         chain postrouting {
>                  type nat hook postrouting priority 0;
>         }
> }
>
> With the above rules, data[priv->sreg_proto_min].data[0] has value 0x160000
> instead of 0x16 on powerpc (Big Endian byte order)
>
> Due to this, mr.range[0].min.all gets assigned value 0 instead of 22.
>
> Below patch fixes it, but it is maybe not the proper way to fix it, so I let
> it up to you.
>
> Christophe
>
> diff --git a/net/ipv4/netfilter/nft_redir_ipv4.c
> b/net/ipv4/netfilter/nft_redir_ipv4.c
> index 643c596..554bb32 100644
> --- a/net/ipv4/netfilter/nft_redir_ipv4.c
> +++ b/net/ipv4/netfilter/nft_redir_ipv4.c
> @@ -28,9 +28,9 @@ static void nft_redir_ipv4_eval(const struct nft_expr
> *expr,
>         memset(&mr, 0, sizeof(mr));
>         if (priv->sreg_proto_min) {
>                 mr.range[0].min.all = (__force __be16)
> -                                       data[priv->sreg_proto_min].data[0];
> +
> *(__be16*)&data[priv->sreg_proto_min].data[0];
>                 mr.range[0].max.all = (__force __be16)
> -                                       data[priv->sreg_proto_max].data[0];
> +
> *(__be16*)&data[priv->sreg_proto_max].data[0];
>                 mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
>         }
>

With nft_nat and nft_redir_ipv6, the three code are almost the same:

http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/netfilter/nft_nat.c
http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/ipv6/netfilter/nft_redir_ipv6.c

Since it seems the same issue may appear, would you like to patch all of them?

regards.

-- 
Arturo Borrero González
--
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

Re: bug : nft_redirect port byteorder issue

[leroy christophe]

Le 12/12/2014 11:49, Arturo Borrero Gonzalez a écrit :
> With nft_nat and nft_redir_ipv6, the three code are almost the same:
>
> http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/netfilter/nft_nat.c
> http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/ipv6/netfilter/nft_redir_ipv6.c
>
> Since it seems the same issue may appear, would you like to patch all of them?
>
> regards.
Hi,

Yes the issue is most likely the same, so I believe it should also be 
fixed there.

Christophe
--
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

Re: bug : nft_redirect port byteorder issue

[Arturo Borrero Gonzalez]

On 12 December 2014 at 12:07, leroy christophe <christophe.leroy@c-s.fr> wrote:
>
> Le 12/12/2014 11:49, Arturo Borrero Gonzalez a écrit :
>>
>> With nft_nat and nft_redir_ipv6, the three code are almost the same:
>>
>>
>> http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/netfilter/nft_nat.c
>>
>> http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/ipv6/netfilter/nft_redir_ipv6.c
>>
>> Since it seems the same issue may appear, would you like to patch all of
>> them?
>>
>> regards.
>
> Hi,
>
> Yes the issue is most likely the same, so I believe it should also be fixed
> there.
>

BTW, please send your patches to netfilter-devel. No need to CC
netfilter@vger.kernel.org.
Patches should include the Signed-off-by line (please be sure they
apply with git am).

Thanks, regards.

-- 
Arturo Borrero González
--
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

Re: bug : nft_redirect port byteorder issue

[leroy christophe]

Le 12/12/2014 12:55, Arturo Borrero Gonzalez a écrit :
> On 12 December 2014 at 12:07, leroy christophe <christophe.leroy@c-s.fr> wrote:
>> Le 12/12/2014 11:49, Arturo Borrero Gonzalez a écrit :
>>> With nft_nat and nft_redir_ipv6, the three code are almost the same:
>>>
>>>
>>> http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/netfilter/nft_nat.c
>>>
>>> http://git.kernel.org/cgit/linux/kernel/git/pablo/nf-next.git/tree/net/ipv6/netfilter/nft_redir_ipv6.c
>>>
>>> Since it seems the same issue may appear, would you like to patch all of
>>> them?
>>>
>>> regards.
>> Hi,
>>
>> Yes the issue is most likely the same, so I believe it should also be fixed
>> there.
>>
> BTW, please send your patches to netfilter-devel. No need to CC
> netfilter@vger.kernel.org.
> Patches should include the Signed-off-by line (please be sure they
> apply with git am).
>
> Thanks, regards.
>
I'm not sure what I proposed it the correct patch, maybe it shall be 
fixed earlier in the chain, I don't know.
So I prefer you or Pablo look at it and do what's best.

Regards
Christophe


--
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

Re: bug : nft_redirect port byteorder issue

[Patrick McHardy]

On 12.12, leroy christophe wrote:
> Le 12/12/2014 12:55, Arturo Borrero Gonzalez a écrit :
> >On 12 December 2014 at 12:07, leroy christophe <christophe.leroy@c-s.fr> wrote:
> I'm not sure what I proposed it the correct patch, maybe it shall be fixed
> earlier in the chain, I don't know.

Yeah, I'm not so sure myself.

Could you please try what happens if you do:

... tcp dport 222 redir :tcp dport

Which should redirect to the same port, but I'm interested if it
actually does that.
--
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

Re: bug : nft_redirect port byteorder issue

[leroy christophe]

Le 12/12/2014 16:25, Patrick McHardy a écrit :
> On 12.12, leroy christophe wrote:
>> Le 12/12/2014 12:55, Arturo Borrero Gonzalez a écrit :
>>> On 12 December 2014 at 12:07, leroy christophe <christophe.leroy@c-s.fr> wrote:
>> I'm not sure what I proposed it the correct patch, maybe it shall be fixed
>> earlier in the chain, I don't know.
> Yeah, I'm not so sure myself.
>
> Could you please try what happens if you do:
>
> ... tcp dport 222 redir :tcp dport
>
> Which should redirect to the same port, but I'm interested if it
> actually does that.
>
Without my patch, I get the following. Note the strange value in the DPT 
on the second line.

[   61.377273] redirIN=eth0 OUT= 
MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37 
DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=29122 DF PROTO=TCP 
SPT=55626 DPT=222 WINDOW=14600 RES=0x00 SYN URGP=0
[   61.377816] rejected IN=eth0 OUT= 
MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37 
DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=29122 DF PROTO=TCP 
SPT=55626 DPT=20 WINDOW=14600 RES=0x00 SYN URGP=0


With my patch, I get correct port.

[  511.994597] redirIN=eth0 OUT= 
MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37 
DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=21300 DF PROTO=TCP 
SPT=55622 DPT=222 WINDOW=14600 RES=0x00 SYN URGP=0
[  511.994999] rejected IN=eth0 OUT= 
MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37 
DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=21300 DF PROTO=TCP 
SPT=55622 DPT=222 WINDOW=14600 RES=0x00 SYN URGP=0


--
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

Re: bug : nft_redirect port byteorder issue

[Patrick McHardy]

On 12.12, leroy christophe wrote:
> Le 12/12/2014 16:25, Patrick McHardy a écrit :
> >On 12.12, leroy christophe wrote:
> >>Le 12/12/2014 12:55, Arturo Borrero Gonzalez a écrit :
> >>>On 12 December 2014 at 12:07, leroy christophe <christophe.leroy@c-s.fr> wrote:
> >>I'm not sure what I proposed it the correct patch, maybe it shall be fixed
> >>earlier in the chain, I don't know.
> >Yeah, I'm not so sure myself.
> >
> >Could you please try what happens if you do:
> >
> >... tcp dport 222 redir :tcp dport
> >
> >Which should redirect to the same port, but I'm interested if it
> >actually does that.
> >
> Without my patch, I get the following. Note the strange value in the DPT on
> the second line.
> 
> [   61.377273] redirIN=eth0 OUT=
> MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37
> DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=29122 DF PROTO=TCP
> SPT=55626 DPT=222 WINDOW=14600 RES=0x00 SYN URGP=0
> [   61.377816] rejected IN=eth0 OUT=
> MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37
> DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=29122 DF PROTO=TCP
> SPT=55626 DPT=20 WINDOW=14600 RES=0x00 SYN URGP=0

Strange, not sure why it is 20.

> With my patch, I get correct port.
> 
> [  511.994597] redirIN=eth0 OUT=
> MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37
> DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=21300 DF PROTO=TCP
> SPT=55622 DPT=222 WINDOW=14600 RES=0x00 SYN URGP=0
> [  511.994999] rejected IN=eth0 OUT=
> MAC=08:00:51:20:44:5b:08:00:27:fe:42:1e:08:00 SRC=172.25.231.37
> DST=172.25.231.5 LEN=60 TOS=0x00 PREC=0x00 TTL=64 ID=21300 DF PROTO=TCP
> SPT=55622 DPT=222 WINDOW=14600 RES=0x00 SYN URGP=0

Thanks! I'll have another look later, but it seems your patch is fine.
--
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

Re: bug : nft_redirect port byteorder issue

[Pablo Neira Ayuso]

On Fri, Dec 12, 2014 at 11:16:29AM +0100, leroy christophe wrote:
> Hi,
> 
> table ip nat {
>         chain prerouting {
>                  type nat hook prerouting priority 0;
>                  tcp dport 222 redirect :22
>         }
>         chain postrouting {
>                  type nat hook postrouting priority 0;
>         }
> }
> 
> With the above rules, data[priv->sreg_proto_min].data[0] has value
> 0x160000 instead of 0x16 on powerpc (Big Endian byte order)
> 
> Due to this, mr.range[0].min.all gets assigned value 0 instead of 22.
> 
> Below patch fixes it, but it is maybe not the proper way to fix it,
> so I let it up to you.
> 
> Christophe
> 
> diff --git a/net/ipv4/netfilter/nft_redir_ipv4.c
> b/net/ipv4/netfilter/nft_redir_ipv4.c
> index 643c596..554bb32 100644
> --- a/net/ipv4/netfilter/nft_redir_ipv4.c
> +++ b/net/ipv4/netfilter/nft_redir_ipv4.c
> @@ -28,9 +28,9 @@ static void nft_redir_ipv4_eval(const struct
> nft_expr *expr,
>  	memset(&mr, 0, sizeof(mr));
>  	if (priv->sreg_proto_min) {
>  		mr.range[0].min.all = (__force __be16)
> -					data[priv->sreg_proto_min].data[0];
> +					*(__be16*)&data[priv->sreg_proto_min].data[0];
>  		mr.range[0].max.all = (__force __be16)
> -					data[priv->sreg_proto_max].data[0];
> +					*(__be16*)&data[priv->sreg_proto_max].data[0];
>  		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
>  	}

It seems userspace was generating the wrong bytecode, so your
workaround was reversing the again the port values.

Please, test the userspace fix I sent you and get back to us.

Thanks for diagnosing!

Re: bug : nft_redirect port byteorder issue

[Patrick McHardy]

Am 22. Dezember 2014 12:54:48 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
>On Fri, Dec 12, 2014 at 11:16:29AM +0100, leroy christophe wrote:
>> Hi,
>> 
>> table ip nat {
>>         chain prerouting {
>>                  type nat hook prerouting priority 0;
>>                  tcp dport 222 redirect :22
>>         }
>>         chain postrouting {
>>                  type nat hook postrouting priority 0;
>>         }
>> }
>> 
>> With the above rules, data[priv->sreg_proto_min].data[0] has value
>> 0x160000 instead of 0x16 on powerpc (Big Endian byte order)
>> 
>> Due to this, mr.range[0].min.all gets assigned value 0 instead of 22.
>> 
>> Below patch fixes it, but it is maybe not the proper way to fix it,
>> so I let it up to you.
>> 
>> Christophe
>> 
>> diff --git a/net/ipv4/netfilter/nft_redir_ipv4.c
>> b/net/ipv4/netfilter/nft_redir_ipv4.c
>> index 643c596..554bb32 100644
>> --- a/net/ipv4/netfilter/nft_redir_ipv4.c
>> +++ b/net/ipv4/netfilter/nft_redir_ipv4.c
>> @@ -28,9 +28,9 @@ static void nft_redir_ipv4_eval(const struct
>> nft_expr *expr,
>>  	memset(&mr, 0, sizeof(mr));
>>  	if (priv->sreg_proto_min) {
>>  		mr.range[0].min.all = (__force __be16)
>> -					data[priv->sreg_proto_min].data[0];
>> +					*(__be16*)&data[priv->sreg_proto_min].data[0];
>>  		mr.range[0].max.all = (__force __be16)
>> -					data[priv->sreg_proto_max].data[0];
>> +					*(__be16*)&data[priv->sreg_proto_max].data[0];
>>  		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
>>  	}
>
>It seems userspace was generating the wrong bytecode, so your
>workaround was reversing the again the port values.
>
>Please, test the userspace fix I sent you and get back to us.

I actually think this is exactly what needs to be done since it also matches what we're doing for runtime gathered data.

>
>Thanks for diagnosing!

Re: bug : nft_redirect port byteorder issue

[Pablo Neira Ayuso]

On Mon, Dec 22, 2014 at 01:44:12PM +0100, Patrick McHardy wrote:
> Am 22. Dezember 2014 12:54:48 MEZ, schrieb Pablo Neira Ayuso <pablo@netfilter.org>:
> >On Fri, Dec 12, 2014 at 11:16:29AM +0100, leroy christophe wrote:
> >> Hi,
> >> 
> >> table ip nat {
> >>         chain prerouting {
> >>                  type nat hook prerouting priority 0;
> >>                  tcp dport 222 redirect :22
> >>         }
> >>         chain postrouting {
> >>                  type nat hook postrouting priority 0;
> >>         }
> >> }
> >> 
> >> With the above rules, data[priv->sreg_proto_min].data[0] has value
> >> 0x160000 instead of 0x16 on powerpc (Big Endian byte order)
> >> 
> >> Due to this, mr.range[0].min.all gets assigned value 0 instead of 22.
> >> 
> >> Below patch fixes it, but it is maybe not the proper way to fix it,
> >> so I let it up to you.
> >> 
> >> Christophe
> >> 
> >> diff --git a/net/ipv4/netfilter/nft_redir_ipv4.c
> >> b/net/ipv4/netfilter/nft_redir_ipv4.c
> >> index 643c596..554bb32 100644
> >> --- a/net/ipv4/netfilter/nft_redir_ipv4.c
> >> +++ b/net/ipv4/netfilter/nft_redir_ipv4.c
> >> @@ -28,9 +28,9 @@ static void nft_redir_ipv4_eval(const struct
> >> nft_expr *expr,
> >>  	memset(&mr, 0, sizeof(mr));
> >>  	if (priv->sreg_proto_min) {
> >>  		mr.range[0].min.all = (__force __be16)
> >> -					data[priv->sreg_proto_min].data[0];
> >> +					*(__be16*)&data[priv->sreg_proto_min].data[0];
> >>  		mr.range[0].max.all = (__force __be16)
> >> -					data[priv->sreg_proto_max].data[0];
> >> +					*(__be16*)&data[priv->sreg_proto_max].data[0];
> >>  		mr.range[0].flags |= NF_NAT_RANGE_PROTO_SPECIFIED;
> >>  	}
> >
> >It seems userspace was generating the wrong bytecode, so your
> >workaround was reversing the again the port values.
> >
> >Please, test the userspace fix I sent you and get back to us.
> 
> I actually think this is exactly what needs to be done since it also
> matches what we're doing for runtime gathered data.

Sure, I just sent a new kernel patch to rectify.