[PATCH nf v3] netfilter: x_tables: perform more sanity tests on rule set

2016-03-22 Thread Florian Westphal 
3rd iteration.

In addition to the problem reported by Ben Hawkes this also adds
a few checks to better validate ->next_offset and the target.

I checked that ip(6)tables-restore still works w. simple rulesets.

The reproducer doesn't work anymore w. patch #4 applied.

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 5/5] netfilter: x_tables: don't move to non-existant next rule

2016-03-22 Thread Florian Westphal 
Ben Hawkes reported an out-of-bounds write in mark_source_chains().
This was caused by improper underflow check -- we should have bailed
earlier.

The underflow check has been fixed in the preceeding change
("netfilter: x_tables: fix unconditional helper").

Just to be safe also add checks to mark_source_chains() in case we have other
bugs that would cause such a condition.

Signed-off-by: Florian Westphal 
---
 net/ipv4/netfilter/arp_tables.c | 8 +---
 net/ipv4/netfilter/ip_tables.c  | 4 
 net/ipv6/netfilter/ip6_tables.c | 4 
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index a2002ff..13266f4 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -439,6 +439,8 @@ static int mark_source_chains(const struct xt_table_info 
*newinfo,
size = e->next_offset;
e = (struct arpt_entry *)
(entry0 + pos + size);
+   if (pos + size >= newinfo->size)
+   return 0;
e->counters.pcnt = pos;
pos += size;
} else {
@@ -461,6 +463,8 @@ static int mark_source_chains(const struct xt_table_info 
*newinfo,
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
+   if (newpos >= newinfo->size)
+   return 0;
}
e = (struct arpt_entry *)
(entry0 + newpos);
@@ -682,10 +686,8 @@ static int translate_table(struct xt_table_info *newinfo, 
void *entry0,
}
}
 
-   if (!mark_source_chains(newinfo, repl->valid_hooks, entry0)) {
-   duprintf("Looping hook\n");
+   if (!mark_source_chains(newinfo, repl->valid_hooks, entry0))
return -ELOOP;
-   }
 
/* Finally, each sanity check must pass */
i = 0;
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 45b1d97..c4836f0 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -520,6 +520,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
size = e->next_offset;
e = (struct ipt_entry *)
(entry0 + pos + size);
+   if (WARN_ON(pos + size >= newinfo->size))
+   return 0;
e->counters.pcnt = pos;
pos += size;
} else {
@@ -541,6 +543,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
+   if (WARN_ON(newpos >= newinfo->size))
+   return 0;
}
e = (struct ipt_entry *)
(entry0 + newpos);
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 85c0942..ab7cdbf 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -532,6 +532,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
size = e->next_offset;
e = (struct ip6t_entry *)
(entry0 + pos + size);
+   if (pos + size >= newinfo->size)
+   return 0;
e->counters.pcnt = pos;
pos += size;
} else {
@@ -553,6 +555,8 @@ mark_source_chains(const struct xt_table_info *newinfo,
} else {
/* ... this is a fallthru */
newpos = pos + e->next_offset;
+   if (newpos >= newinfo->size)
+   return 0;
}
e = (struct ip6t_entry *)
(entry0 + newpos);
-- 
2.4.10

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/5] netfilter: x_tables: validate e->target_offset early

2016-03-22 Thread Florian Westphal 
We should check that e->target_offset is sane before
mark_source_chains gets called since it will fetch the target entry
for loop detection.

Signed-off-by: Florian Westphal 
---
 net/ipv4/netfilter/arp_tables.c | 17 -
 net/ipv4/netfilter/ip_tables.c  | 17 -
 net/ipv6/netfilter/ip6_tables.c | 17 -
 3 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index bf08192..830bbe8 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -474,14 +474,12 @@ next:
return 1;
 }
 
-static inline int check_entry(const struct arpt_entry *e, const char *name)
+static inline int check_entry(const struct arpt_entry *e)
 {
const struct xt_entry_target *t;
 
-   if (!arp_checkentry(>arp)) {
-   duprintf("arp_tables: arp check failed %p %s.\n", e, name);
+   if (!arp_checkentry(>arp))
return -EINVAL;
-   }
 
if (e->target_offset + sizeof(struct xt_entry_target) > e->next_offset)
return -EINVAL;
@@ -522,10 +520,6 @@ find_check_entry(struct arpt_entry *e, const char *name, 
unsigned int size)
struct xt_target *target;
int ret;
 
-   ret = check_entry(e, name);
-   if (ret)
-   return ret;
-
e->counters.pcnt = xt_percpu_counter_alloc();
if (IS_ERR_VALUE(e->counters.pcnt))
return -ENOMEM;
@@ -576,6 +570,7 @@ static inline int check_entry_size_and_hooks(struct 
arpt_entry *e,
 unsigned int valid_hooks)
 {
unsigned int h;
+   int err;
 
if ((unsigned long)e % __alignof__(struct arpt_entry) != 0 ||
(unsigned char *)e + sizeof(struct arpt_entry) >= limit) {
@@ -590,6 +585,10 @@ static inline int check_entry_size_and_hooks(struct 
arpt_entry *e,
return -EINVAL;
}
 
+   err = check_entry(e);
+   if (err)
+   return err;
+
/* Check hooks & underflows */
for (h = 0; h < NF_ARP_NUMHOOKS; h++) {
if (!(valid_hooks & (1 << h)))
@@ -1246,7 +1245,7 @@ check_compat_entry_size_and_hooks(struct 
compat_arpt_entry *e,
}
 
/* For purposes of check_entry casting the compat entry is fine */
-   ret = check_entry((struct arpt_entry *)e, name);
+   ret = check_entry((struct arpt_entry *)e);
if (ret)
return ret;
 
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index e53f8d6..1d72a3c 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -569,14 +569,12 @@ static void cleanup_match(struct xt_entry_match *m, 
struct net *net)
 }
 
 static int
-check_entry(const struct ipt_entry *e, const char *name)
+check_entry(const struct ipt_entry *e)
 {
const struct xt_entry_target *t;
 
-   if (!ip_checkentry(>ip)) {
-   duprintf("ip check failed %p %s.\n", e, name);
+   if (!ip_checkentry(>ip))
return -EINVAL;
-   }
 
if (e->target_offset + sizeof(struct xt_entry_target) >
e->next_offset)
@@ -666,10 +664,6 @@ find_check_entry(struct ipt_entry *e, struct net *net, 
const char *name,
struct xt_mtchk_param mtpar;
struct xt_entry_match *ematch;
 
-   ret = check_entry(e, name);
-   if (ret)
-   return ret;
-
e->counters.pcnt = xt_percpu_counter_alloc();
if (IS_ERR_VALUE(e->counters.pcnt))
return -ENOMEM;
@@ -741,6 +735,7 @@ check_entry_size_and_hooks(struct ipt_entry *e,
   unsigned int valid_hooks)
 {
unsigned int h;
+   int err;
 
if ((unsigned long)e % __alignof__(struct ipt_entry) != 0 ||
(unsigned char *)e + sizeof(struct ipt_entry) >= limit) {
@@ -755,6 +750,10 @@ check_entry_size_and_hooks(struct ipt_entry *e,
return -EINVAL;
}
 
+   err = check_entry(e);
+   if (err)
+   return err;
+
/* Check hooks & underflows */
for (h = 0; h < NF_INET_NUMHOOKS; h++) {
if (!(valid_hooks & (1 << h)))
@@ -1506,7 +1505,7 @@ check_compat_entry_size_and_hooks(struct compat_ipt_entry 
*e,
}
 
/* For purposes of check_entry casting the compat entry is fine */
-   ret = check_entry((struct ipt_entry *)e, name);
+   ret = check_entry((struct ipt_entry *)e);
if (ret)
return ret;
 
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 84f9baf..26a5ad1 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -581,14 +581,12 @@ static void cleanup_match(struct xt_entry_match *m, 
struct net *net)
 }
 
 static int
-check_entry(const struct ip6t_entry *e, const char *name)
+check_entry(const struct ip6t_entry *e)
 {
const struct xt_entry_target

[PATCH 2/5] netfilter: x_tables: make sure e->next_offset covers remaining blob size

2016-03-22 Thread Florian Westphal 
Otherwise this function may read data beyond the ruleset blob.

Signed-off-by: Florian Westphal 
---
 net/ipv4/netfilter/arp_tables.c | 6 --
 net/ipv4/netfilter/ip_tables.c  | 6 --
 net/ipv6/netfilter/ip6_tables.c | 6 --
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 830bbe8..51d4fe5 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -573,7 +573,8 @@ static inline int check_entry_size_and_hooks(struct 
arpt_entry *e,
int err;
 
if ((unsigned long)e % __alignof__(struct arpt_entry) != 0 ||
-   (unsigned char *)e + sizeof(struct arpt_entry) >= limit) {
+   (unsigned char *)e + sizeof(struct arpt_entry) >= limit ||
+   (unsigned char *)e + e->next_offset > limit) {
duprintf("Bad offset %p\n", e);
return -EINVAL;
}
@@ -1232,7 +1233,8 @@ check_compat_entry_size_and_hooks(struct 
compat_arpt_entry *e,
 
duprintf("check_compat_entry_size_and_hooks %p\n", e);
if ((unsigned long)e % __alignof__(struct compat_arpt_entry) != 0 ||
-   (unsigned char *)e + sizeof(struct compat_arpt_entry) >= limit) {
+   (unsigned char *)e + sizeof(struct compat_arpt_entry) >= limit ||
+   (unsigned char *)e + e->next_offset > limit) {
duprintf("Bad offset %p, limit = %p\n", e, limit);
return -EINVAL;
}
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 1d72a3c..fb7694e 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -738,7 +738,8 @@ check_entry_size_and_hooks(struct ipt_entry *e,
int err;
 
if ((unsigned long)e % __alignof__(struct ipt_entry) != 0 ||
-   (unsigned char *)e + sizeof(struct ipt_entry) >= limit) {
+   (unsigned char *)e + sizeof(struct ipt_entry) >= limit ||
+   (unsigned char *)e + e->next_offset > limit) {
duprintf("Bad offset %p\n", e);
return -EINVAL;
}
@@ -1492,7 +1493,8 @@ check_compat_entry_size_and_hooks(struct compat_ipt_entry 
*e,
 
duprintf("check_compat_entry_size_and_hooks %p\n", e);
if ((unsigned long)e % __alignof__(struct compat_ipt_entry) != 0 ||
-   (unsigned char *)e + sizeof(struct compat_ipt_entry) >= limit) {
+   (unsigned char *)e + sizeof(struct compat_ipt_entry) >= limit ||
+   (unsigned char *)e + e->next_offset > limit) {
duprintf("Bad offset %p, limit = %p\n", e, limit);
return -EINVAL;
}
diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
index 26a5ad1..b248528f 100644
--- a/net/ipv6/netfilter/ip6_tables.c
+++ b/net/ipv6/netfilter/ip6_tables.c
@@ -750,7 +750,8 @@ check_entry_size_and_hooks(struct ip6t_entry *e,
int err;
 
if ((unsigned long)e % __alignof__(struct ip6t_entry) != 0 ||
-   (unsigned char *)e + sizeof(struct ip6t_entry) >= limit) {
+   (unsigned char *)e + sizeof(struct ip6t_entry) >= limit ||
+   (unsigned char *)e + e->next_offset > limit) {
duprintf("Bad offset %p\n", e);
return -EINVAL;
}
@@ -1504,7 +1505,8 @@ check_compat_entry_size_and_hooks(struct 
compat_ip6t_entry *e,
 
duprintf("check_compat_entry_size_and_hooks %p\n", e);
if ((unsigned long)e % __alignof__(struct compat_ip6t_entry) != 0 ||
-   (unsigned char *)e + sizeof(struct compat_ip6t_entry) >= limit) {
+   (unsigned char *)e + sizeof(struct compat_ip6t_entry) >= limit ||
+   (unsigned char *)e + e->next_offset > limit) {
duprintf("Bad offset %p, limit = %p\n", e, limit);
return -EINVAL;
}
-- 
2.4.10

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: net/sctp: stack-out-of-bounds in sctp_getsockopt

2016-03-22 Thread Eric Dumazet 
On Tue, 2016-03-22 at 08:21 -0700, Eric Dumazet wrote:
> On Tue, 2016-03-22 at 23:08 +0800, Baozeng Ding wrote:
> > Hi all,
> > 
> > The following program triggers an out-of-bounds bug in
> > sctp_getsockopt. The kernel version is 4.5 (on Mar 16
> > commit 09fd671ccb2475436bd5f597f751ca4a7d177aea). 
> > 
> > ==
> > BUG: KASAN: stack-out-of-bounds in string+0x1ef/0x200 at addr
> > 88003ae679e0
> > Read of size 1 by task syz-executor/19753
> > page:eaeb99c0 count:0 mapcount:0 mapping:  (null)
> > index:0x0
> > flags: 0x1fffc00()
> > page dumped because: kasan: bad access detected
> > CPU: 3 PID: 19753 Comm: syz-executor Not tainted 4.5.0+ #8
> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> > rel-1.8.2-0-g33fbe13 by qemu-project.org 04/01/2014
> >  0003 88003ae67578 82945051 88003ae67608
> >  88003ae679e0 0096 dc00 88003ae675f8
> >  81709f88 030d  0286
> > Call Trace:
> >  [< inline >] __dump_stack lib/dump_stack.c:15
> >  [] dump_stack+0xb3/0x112 lib/dump_stack.c:51
> >  [< inline >] print_address_description mm/kasan/report.c:150
> >  [] kasan_report_error+0x4f8/0x530 mm/kasan/report.c:236
> >  [] ? __lock_acquire+0x15fb/0x5dd0 
> > kernel/locking/lockdep.c:3226
> >  [< inline >] kasan_report mm/kasan/report.c:259
> >  [] __asan_report_load1_noabort+0x3e/0x40 
> > mm/kasan/report.c:277
> >  [] ? string+0x1ef/0x200 lib/vsprintf.c:591
> >  [] string+0x1ef/0x200 lib/vsprintf.c:591
> >  [] vsnprintf+0xb83/0x1900 lib/vsprintf.c:2049
> >  [] ? pointer+0xab0/0xab0 lib/vsprintf.c:1584
> >  [] __request_module+0x132/0x6b0 kernel/kmod.c:146
> >  [] ? mark_held_locks+0xd0/0x130 
> > kernel/locking/lockdep.c:2552
> >  [] ? call_usermodehelper_setup+0x2b0/0x2b0 
> > kernel/kmod.c:530
> >  [] ? mutex_lock_interruptible_nested+0x980/0x980
> >  [] ? __might_fault+0xe4/0x1d0 mm/memory.c:3833
> >  [] find_inlist_lock.constprop.17+0x10c/0x210 
> > net/bridge/netfilter/ebtables.c:347
> >  [< inline >] find_table_lock net/bridge/netfilter/ebtables.c:356
> >  [] do_ebt_get_ctl+0x13b/0x540 
> > net/bridge/netfilter/ebtables.c:1524
> >  [] ? copy_everything_to_user+0x600/0x600 
> > net/bridge/netfilter/ebtables.c:1455
> >  [< inline >] ? __mutex_unlock_common_slowpath 
> > kernel/locking/mutex.c:751
> >  [] ? __mutex_unlock_slowpath+0x239/0x3f0 
> > kernel/locking/mutex.c:762
> >  [] ? mutex_unlock+0x9/0x10 kernel/locking/mutex.c:437
> >  [] ? nf_sockopt_find+0x1a6/0x220 
> > net/netfilter/nf_sockopt.c:87
> >  [< inline >] nf_sockopt net/netfilter/nf_sockopt.c:103
> >  [] nf_getsockopt+0x6d/0xc0 net/netfilter/nf_sockopt.c:121
> >  [] ip_getsockopt+0x135/0x190 net/ipv4/ip_sockglue.c:1523
> >  [] ? do_ip_getsockopt+0x1520/0x1520 
> > net/ipv4/ip_sockglue.c:1353
> >  [< inline >] ? wake_up_process kernel/sched/core.c:2024
> >  [] ? wake_up_q+0x82/0xe0 kernel/sched/core.c:416
> >  [< inline >] ? atomic_dec_and_test 
> > /arch/x86/include/asm/atomic.h:117
> >  [< inline >] ? mmdrop include/linux/sched.h:2611
> >  [] ? drop_futex_key_refs.isra.13+0x70/0xe0 
> > kernel/futex.c:444
> >  [] sctp_getsockopt+0x18d/0x3f40 net/sctp/socket.c:5964
> >  [] ? __lock_acquire+0x15fb/0x5dd0 
> > kernel/locking/lockdep.c:3226
> >  [] ? sctp_do_peeloff+0x2b0/0x2b0 net/sctp/socket.c:4434
> >  [] ? debug_check_no_locks_freed+0x290/0x290 
> > kernel/locking/lockdep.c:4104
> >  [< inline >] ? rcu_read_unlock include/linux/rcupdate.h:922
> >  [] ? __fget+0x20c/0x3b0 fs/file.c:712
> >  [< inline >] ? rcu_lock_release include/linux/rcupdate.h:491
> >  [< inline >] ? rcu_read_unlock include/linux/rcupdate.h:926
> >  [] ? __fget+0x235/0x3b0 fs/file.c:712
> >  [] ? __fget+0x47/0x3b0 fs/file.c:696
> >  [] ? __fget_light+0xa1/0x1f0 fs/file.c:759
> >  [] sock_common_getsockopt+0x95/0xd0 net/core/sock.c:2579
> >  [< inline >] SYSC_getsockopt net/socket.c:1783
> >  [] SyS_getsockopt+0x142/0x230 net/socket.c:1765
> >  [] ? SyS_setsockopt+0x240/0x240 net/socket.c:1752
> >  [] ? entry_SYSCALL_64_fastpath+0x5/0xc1 
> > arch/x86/entry/entry_64.S:191
> >  [] ? trace_hardirqs_on_thunk+0x17/0x19 
> > arch/x86/entry/thunk_64.S:39
> >  [] entry_SYSCALL_64_fastpath+0x23/0xc1 
> > arch/x86/entry/entry_64.S:207
> > Memory state around the buggy address:
> >  88003ae67880: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >  88003ae67900: 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2 00 00 00
> > >88003ae67980: 00 00 00 00 00 00 00 00 00 00 00 00 f4 f3 f3 f3
> >^
> >  88003ae67a00: f3 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> >  88003ae67a80: f1 f1 f1 f1 04 f4 f4 f4 f3 f3 f3 f3 00 00 00 00
> > ==
> > 
> > #include 
> > #include

Re: [PATCH 4/4 v5] nftables: rule: Change the field "rule->comment" for an nftnl_udata_buf

2016-03-22 Thread Carlos Falgueras García 

On 21/03/16 23:13, Pablo Neira Ayuso wrote:

On Tue, Mar 15, 2016 at 09:28:07PM +0100, Carlos Falgueras García wrote:

Now it is possible to store multiple variable length user data into rule.
Modify the parser in order to fill the nftnl_udata with the comment, and
the print function for extract these commentary and print it to user.

Signed-off-by: Carlos Falgueras García 
---
  include/rule.h|  7 +++
  src/netlink_delinearize.c | 52 +--
  src/netlink_linearize.c   | 16 +--
  3 files changed, 71 insertions(+), 4 deletions(-)

diff --git a/include/rule.h b/include/rule.h
index c848f0f..b52f0ac 100644
--- a/include/rule.h
+++ b/include/rule.h
@@ -4,6 +4,7 @@
  #include 
  #include 
  #include 
+#include 

  /**
   * struct handle - handle for tables, chains, rules and sets
@@ -396,4 +397,10 @@ extern int do_command(struct netlink_ctx *ctx, struct cmd 
*cmd);
  extern int cache_update(enum cmd_ops cmd, struct list_head *msgs);
  extern void cache_release(void);

+enum udata_type {
+   UDATA_TYPE_COMMENT,
+   __UDATA_TYPE_MAX,
+};
+#define UDATA_TYPE_MAX (__UDATA_TYPE_MAX - 1)
+
  #endif /* NFTABLES_RULE_H */
diff --git a/src/netlink_delinearize.c b/src/netlink_delinearize.c
index d431588..5fcb5c1 100644
--- a/src/netlink_delinearize.c
+++ b/src/netlink_delinearize.c
@@ -25,6 +25,7 @@
  #include 
  #include 
  #include 
+#include 

  struct netlink_parse_ctx {
struct list_head*msgs;
@@ -1746,6 +1747,54 @@ static void rule_parse_postprocess(struct 
netlink_parse_ctx *ctx, struct rule *r
}
  }

+static int parse_udata_cb(const struct nftnl_udata *attr, void *data)
+{
+   unsigned char *value = nftnl_udata_attr_value(attr);
+   uint8_t type = nftnl_udata_attr_type(attr);
+   uint8_t len = nftnl_udata_attr_len(attr);
+   const struct nftnl_udata **tb = data;
+
+   switch (type) {
+   case UDATA_TYPE_COMMENT:
+   if (value[len - 1] != '\0')
+   return -1;
+   break;
+   default:
+   break;
+   };
+
+   tb[type] = attr;
+   return 1;
+}
+
+static char *udata_get_comment(const void *data, uint32_t data_len)
+{
+   const struct nftnl_udata *tb[UDATA_TYPE_MAX + 1] = {};
+   struct nftnl_udata_buf *udata;
+   uint8_t attr_len;
+   char *comment = NULL;
+
+   udata = nftnl_udata_alloc(data_len);
+   if (!udata)
+   memory_allocation_error();
+   nftnl_udata_copy_data(udata, data, data_len);
+
+   if (nftnl_udata_parse(udata, parse_udata_cb, tb) <= 0)
+   goto exit;


I think this should be instead:

if (nftnl_udata_parse(data, data_len, parse_udata_cb, tb) <= 0)

So you don't need to allocate the buffer then copy data into it.

I think the buffer infrastructure is only necessary to build the TLV
attributes, not to parse it.



Ok.


+   if (!tb[UDATA_TYPE_COMMENT])
+   goto exit;
+
+   attr_len = nftnl_udata_attr_len(tb[UDATA_TYPE_COMMENT]);
+   comment = xmalloc(attr_len);
+   memcpy(comment, nftnl_udata_attr_value(tb[UDATA_TYPE_COMMENT]),
+  attr_len);


I'd suggest:

 comment = xstrdup(nftnl_udata_attr_get_str(tb[UDATA_TYPE_COMMENT]));


I'll change this.


+
+exit:
+   nftnl_udata_free(udata);
+   return comment;
+}
+
  struct rule *netlink_delinearize_rule(struct netlink_ctx *ctx,
  const struct nftnl_rule *nlr)
  {

--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4 v5] libnftnl: Implement new buffer of TLV objects

2016-03-22 Thread Carlos Falgueras García 

On 21/03/16 23:10, Pablo Neira Ayuso wrote:

On Tue, Mar 15, 2016 at 09:28:04PM +0100, Carlos Falgueras García wrote:

These functions allow to create a buffer (nftnl_udata_buf) of TLV objects
(nftnl_udata). It is inspired by libmnl/src/attr.c. It can be used to store
several variable length user data into an object.

Example usage:
```
struct nftnl_udata_buf *buf;
struct nftnl_udata *attr;
const char str[] = "Hello World!";

buf = nftnl_udata_alloc(UDATA_SIZE);
if (!buf) {
perror("OOM");
exit(EXIT_FAILURE);
}

if (!nftnl_udata_put_strz(buf, MY_TYPE, str)) {
perror("Can't put attribute \"%s\"", str);
exit(EXIT_FAILURE);
}

nftnl_udata_for_each(buf, attr) {
printf("%s\n", (char *)nftnl_udata_attr_value(attr));
}


u>   nftnl_udata_free(buf);

```


I don't understand what that means :/


Signed-off-by: Carlos Falgueras García 
---
  include/Makefile.am  |   1 +
  include/libnftnl/Makefile.am |   1 +
  include/libnftnl/udata.h |  49 
  include/udata.h  |  40 +
  src/Makefile.am  |   1 +
  src/libnftnl.map |  32 +++
  src/udata.c  | 133 +++
  7 files changed, 257 insertions(+)
  create mode 100644 include/libnftnl/udata.h
  create mode 100644 include/udata.h
  create mode 100644 src/udata.c

diff --git a/include/Makefile.am b/include/Makefile.am
index be9eb9b..9f55737 100644
--- a/include/Makefile.am
+++ b/include/Makefile.am
@@ -12,4 +12,5 @@ noinst_HEADERS = internal.h   \
 expr.h \
 json.h \
 set_elem.h \
+udata.h\
 utils.h
diff --git a/include/libnftnl/Makefile.am b/include/libnftnl/Makefile.am
index 84f01b6..457ec95 100644
--- a/include/libnftnl/Makefile.am
+++ b/include/libnftnl/Makefile.am
@@ -7,4 +7,5 @@ pkginclude_HEADERS = batch.h\
 set.h  \
 ruleset.h  \
 common.h   \
+udata.h\
 gen.h
diff --git a/include/libnftnl/udata.h b/include/libnftnl/udata.h
new file mode 100644
index 000..f65a1dc
--- /dev/null
+++ b/include/libnftnl/udata.h
@@ -0,0 +1,49 @@
+#ifndef _LIBNFTNL_UDATA_H_
+#define _LIBNFTNL_UDATA_H_
+
+#include 
+#include 
+#include 
+
+/*
+ * nftnl user data attributes API
+ */
+struct nftnl_udata;
+struct nftnl_udata_buf;
+
+/* nftnl_udata_buf */
+struct nftnl_udata_buf *nftnl_udata_alloc(uint32_t data_size);
+void nftnl_udata_free(struct nftnl_udata_buf *buf);


If nftnl_udata_alloc() returns struct nftnl_udata_buf, then it's
better to call this:

 nftnl_udata_buf_alloc()

instead.


+uint32_t nftnl_udata_len(const struct nftnl_udata_buf *buf);
+uint32_t nftnl_udata_size(const struct nftnl_udata_buf *buf);
+void *nftnl_udata_data(const struct nftnl_udata_buf *buf);


Same thing for these functions: nftnl_udata_buf_*().


+void nftnl_udata_copy_data(struct nftnl_udata_buf *buf, const void *data,
+  uint32_t len);


Please, rename this to:

 nftnl_udata_buf_put()



Ok.


+struct nftnl_udata *nftnl_udata_start(const struct nftnl_udata_buf *buf);
+struct nftnl_udata *nftnl_udata_end(const struct nftnl_udata_buf *buf);
+
+/* putters */
+bool nftnl_udata_put(struct nftnl_udata_buf *buf, uint8_t type, uint32_t len,
+const void *value);
+bool nftnl_udata_put_strz(struct nftnl_udata_buf *buf, uint8_t type,
+ const char *strz);
+
+/* nftnl_udata_attr */
+uint8_t nftnl_udata_attr_type(const struct nftnl_udata *attr);
+uint8_t nftnl_udata_attr_len(const struct nftnl_udata *attr);
+void *nftnl_udata_attr_value(const struct nftnl_udata *attr);
+
+/* iterator */
+struct nftnl_udata *nftnl_udata_attr_next(const struct nftnl_udata *attr);
+
+#define nftnl_udata_for_each(buf, attr)   \
+   for ((attr) = nftnl_udata_start(buf); \
+(char *)(nftnl_udata_end(buf)) > (char *)(attr); \
+(attr) = nftnl_udata_attr_next(attr))
+
+typedef int (*nftnl_udata_cb_t)(const struct nftnl_udata *attr,
+   void *data);
+int nftnl_udata_parse(const struct nftnl_udata_buf *buf, nftnl_udata_cb_t cb,
+ void *data);
+
+#endif /* _LIBNFTNL_UDATA_H_ */
diff --git a/include/udata.h b/include/udata.h
new file mode 100644
index 000..407a3b9
--- /dev/null
+++ b/include/udata.h
@@ -0,0 +1,40 @@
+#ifndef _LIBNFTNL_UDATA_INTERNAL_H_
+#define _LIBNFTNL_UDATA_INTERNAL_H_
+
+#include 
+#include 
+
+/*
+ * TLV structures:
+ * nftnl_udata
+ *  < HEADER > <-- PAYLOAD -->
+ * ++-+- - - - - - - - - - - -+
+ * |

Re: [PATCH] openvswitch: Fix checking for new expected connections.

2016-03-22 Thread Pablo Neira Ayuso 
On Mon, Mar 21, 2016 at 11:15:19AM -0700, Jarno Rajahalme wrote:
> OVS should call into CT NAT for packets of new expected connections only
> when the conntrack state is persisted with the 'commit' option to the
> OVS CT action.  The test for this condition is doubly wrong, as the CT
> status field is ANDed with the bit number (IPS_EXPECTED_BIT) rather
> than the mask (IPS_EXPECTED), and due to the wrong assumption that the
> expected bit would apply only for the first (i.e., 'new') packet of a
> connection, while in fact the expected bit remains on for the lifetime of
> an expected connection.  The 'ctinfo' value IP_CT_RELATED derived from
> the ct status can be used instead, as it is only ever applicable to
> the 'new' packets of the expected connection.

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html