[PATCH] src: netlink_delinearize: Fix datatype for len

[PATCH] src: netlink_delinearize: Fix datatype for len

[Shivani Bhardwaj]

Change the data type of len from unsigned int to int in order to make
it valid for checks like

if (len < 0)

The issue was brought into attention by the unexplained behavior of
frag with frag-off. Bugzilla entry:
https://bugzilla.netfilter.org/show_bug.cgi?id=935

This patch fixes this bug, however there are still issues with frag
that need to be fixed.

Signed-off-by: Shivani Bhardwaj <shivanib134@gmail.com>
---
 src/netlink_delinearize.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index ae6abb0..2d7a417 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -107,7 +107,7 @@ static void netlink_release_registers(struct netlink_parse_ctx *ctx)
 static struct expr *netlink_parse_concat_expr(struct netlink_parse_ctx *ctx,
 					      const struct location *loc,
 					      unsigned int reg,
-					      unsigned int len)
+					      int len)
 {
 	struct expr *concat, *expr;
 
@@ -134,7 +134,7 @@ err:
 static struct expr *netlink_parse_concat_data(struct netlink_parse_ctx *ctx,
 					      const struct location *loc,
 					      unsigned int reg,
-					      unsigned int len,
+					      int len,
 					      struct expr *data)
 {
 	struct expr *concat, *expr, *i;
-- 
1.9.1

Re: [PATCH] src: netlink_delinearize: Fix datatype for len

[Florian Westphal]

Shivani Bhardwaj <shivanib134@gmail.com> wrote:
> Change the data type of len from unsigned int to int in order to make
> it valid for checks like
> 
> if (len < 0)
> 
> The issue was brought into attention by the unexplained behavior of
> frag with frag-off. Bugzilla entry:
> https://bugzilla.netfilter.org/show_bug.cgi?id=935
> 
> This patch fixes this bug, however there are still issues with frag
> that need to be fixed.

exthdr (frag) seems to have several issues:

- we should reject exthdr and only allow it with ipv6.
- for inet/bridge, we should also inject ipv6 dependency
- some exthdrs (frag for instance) have odd bit lengths
  and need mask/shift instructions.

For example, in your example rule we generate:
   [ exthdr load 1b @ 44 + 2 => reg 1 ]
   [ cmp eq reg 1 0x00002100 ]

But thats not correct -- we truncated the load to one byte.
Instead we should have loaded 2 bytes and then masked off the extra 3bits.

I'll work on this.

Re: [PATCH] src: netlink_delinearize: Fix datatype for len

[Shivani Bhardwaj]

On Mon, Feb 29, 2016 at 3:36 PM, Florian Westphal <fw@strlen.de> wrote:
> Shivani Bhardwaj <shivanib134@gmail.com> wrote:
>> Change the data type of len from unsigned int to int in order to make
>> it valid for checks like
>>
>> if (len < 0)
>>
>> The issue was brought into attention by the unexplained behavior of
>> frag with frag-off. Bugzilla entry:
>> https://bugzilla.netfilter.org/show_bug.cgi?id=935
>>
>> This patch fixes this bug, however there are still issues with frag
>> that need to be fixed.
>
> exthdr (frag) seems to have several issues:
>
> - we should reject exthdr and only allow it with ipv6.
> - for inet/bridge, we should also inject ipv6 dependency
> - some exthdrs (frag for instance) have odd bit lengths
>   and need mask/shift instructions.
>
> For example, in your example rule we generate:
>    [ exthdr load 1b @ 44 + 2 => reg 1 ]
>    [ cmp eq reg 1 0x00002100 ]
>
> But thats not correct -- we truncated the load to one byte.
> Instead we should have loaded 2 bytes and then masked off the extra 3bits.
>
> I'll work on this.

In the chain this rule shows up as

chain input {
        type filter hook input priority 0; policy accept;
        frag unknown 0x0 [invalid type]
    }

This is also the case with some icmpv6 options (id and max-delay),
please take a note of this too.
Thanks.