[PATCH V3] netfilter: x_tables: Fix use-after-free in ipt_do_table.

2017-07-26 Thread Taehee Yoo 
If verdict is NF_STOLEN in the SYNPROXY target,
the skb is consumed.
However, ipt_do_table() always tries to get ip header from the skb.
So that, KASAN triggers the use-after-free message.

We can reproduce this message using below command.
  # iptables -I INPUT -p tcp -j SYNPROXY --mss 1460

[ 193.542265] BUG: KASAN: use-after-free in ipt_do_table+0x1405/0x1c10
[ ... ]
[ 193.578603] Call Trace:
[ 193.581590] 
[ 193.584107] dump_stack+0x68/0xa0
[ 193.588168] print_address_description+0x78/0x290
[ 193.593828] ? ipt_do_table+0x1405/0x1c10
[ 193.598690] kasan_report+0x230/0x340
[ 193.603194] __asan_report_load2_noabort+0x19/0x20
[ 193.608950] ipt_do_table+0x1405/0x1c10
[ 193.613591] ? rcu_read_lock_held+0xae/0xd0
[ 193.618631] ? ip_route_input_rcu+0x27d7/0x4270
[ 193.624348] ? ipt_do_table+0xb68/0x1c10
[ 193.629124] ? do_add_counters+0x620/0x620
[ 193.634234] ? iptable_filter_net_init+0x60/0x60
[ ... ]

After this patch, only when verdict is XT_CONTINUE,
ipt_do_table() tries to get ip header.
Also arpt_do_table() is modified because it has same bug.

Signed-off-by: Taehee Yoo 
---
V3:
 - Modified arpt_do_table() is added.

V2:
 - Change commit log message.

V1:
 - Initial Version

 net/ipv4/netfilter/arp_tables.c | 10 +-
 net/ipv4/netfilter/ip_tables.c  |  9 +
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
index 0bc3c3d..9e9d9af 100644
--- a/net/ipv4/netfilter/arp_tables.c
+++ b/net/ipv4/netfilter/arp_tables.c
@@ -268,14 +268,14 @@ unsigned int arpt_do_table(struct sk_buff *skb,
acpar.targinfo = t->data;
verdict = t->u.kernel.target->target(skb, &acpar);
 
-   /* Target might have changed stuff. */
-   arp = arp_hdr(skb);
-
-   if (verdict == XT_CONTINUE)
+   if (verdict == XT_CONTINUE) {
+   /* Target might have changed stuff. */
+   arp = arp_hdr(skb);
e = arpt_next_entry(e);
-   else
+   } else {
/* Verdict */
break;
+   }
} while (!acpar.hotdrop);
xt_write_recseq_end(addend);
local_bh_enable();
diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 2a55a40..622ed28 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -352,13 +352,14 @@ ipt_do_table(struct sk_buff *skb,
acpar.targinfo = t->data;
 
verdict = t->u.kernel.target->target(skb, &acpar);
-   /* Target might have changed stuff. */
-   ip = ip_hdr(skb);
-   if (verdict == XT_CONTINUE)
+   if (verdict == XT_CONTINUE) {
+   /* Target might have changed stuff. */
+   ip = ip_hdr(skb);
e = ipt_next_entry(e);
-   else
+   } else {
/* Verdict */
break;
+   }
} while (!acpar.hotdrop);
 
xt_write_recseq_end(addend);
-- 
2.9.3

--
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 V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

2017-07-26 Thread Taehee Yoo 
2017-07-26 20:06 GMT+09:00 Pablo Neira Ayuso :
> On Wed, Jul 26, 2017 at 11:27:16AM +0200, Florian Westphal wrote:
>> Taehee Yoo  wrote:
>> > If verdict is NF_STOLEN in the SYNPROXY target,
>> > the skb is consumed.
>> > However, ipt_do_table() always tries to get ip header from the skb.
>> > So that, KASAN triggers the use-after-free message.
>>
>> In case anyone wonders, ip6tables doesn't have this problem
>> because we pass *skb, not ip6hdr to ip6_packet_match().
>
> I think it would be good to make these code converge to what ip6tables
> is doing while fixing up this?
>
>> arptables has the same bug, it seems (no target returns STOLEN,
>> but I think we should fix it there as well).
>
> Yes, even if no target returns what triggers the problem, it's good if
> we fix this now so we make sure whatever new extension gets in in the
> future works accordingly.
>
> Thanks!

Thank you for reviews!
I will send the V3 patch that includes modified arpt_do_table()
that is reviewed point.
--
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

[nft PATCH] mnl: Remove dead code

2017-07-26 Thread Phil Sutter 
Remove the functions mnl_nft_rule_add() and *_delete() since they are
not used throughout the code. Commit a72315d2bad47 ("src: add rule
batching support") changed their only caller to use the batch variant
introduced at the same time.

Signed-off-by: Phil Sutter 
---
 include/mnl.h |  4 
 src/mnl.c | 28 
 2 files changed, 32 deletions(-)

diff --git a/include/mnl.h b/include/mnl.h
index 9f5b34f60d023..3d2d7fef93ba2 100644
--- a/include/mnl.h
+++ b/include/mnl.h
@@ -33,10 +33,6 @@ int mnl_nft_rule_batch_del(struct nftnl_rule *nlr, struct 
nftnl_batch *batch,
 int mnl_nft_rule_batch_replace(struct nftnl_rule *nlr, struct nftnl_batch 
*batch,
   unsigned int flags, uint32_t seqnum);
 
-int mnl_nft_rule_add(struct mnl_socket *nf_sock, struct nftnl_rule *r,
-unsigned int flags);
-int mnl_nft_rule_delete(struct mnl_socket *nf_sock, struct nftnl_rule *r,
-   unsigned int flags);
 struct nftnl_rule_list *mnl_nft_rule_dump(struct mnl_socket *nf_sock,
int family);
 
diff --git a/src/mnl.c b/src/mnl.c
index 7639312381507..3db80de6da02d 100644
--- a/src/mnl.c
+++ b/src/mnl.c
@@ -339,34 +339,6 @@ int mnl_nft_rule_batch_del(struct nftnl_rule *nlr, struct 
nftnl_batch *batch,
 /*
  * Rule
  */
-int mnl_nft_rule_add(struct mnl_socket *nf_sock, struct nftnl_rule *nlr,
-unsigned int flags)
-{
-   char buf[MNL_SOCKET_BUFFER_SIZE];
-   struct nlmsghdr *nlh;
-
-   nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_NEWRULE,
-   nftnl_rule_get_u32(nlr, NFTNL_RULE_FAMILY),
-   NLM_F_ACK | NLM_F_CREATE | flags, seq);
-   nftnl_rule_nlmsg_build_payload(nlh, nlr);
-
-   return nft_mnl_talk(nf_sock, nlh, nlh->nlmsg_len, NULL, NULL);
-}
-
-int mnl_nft_rule_delete(struct mnl_socket *nf_sock, struct nftnl_rule *nlr,
-   unsigned int flags)
-{
-   char buf[MNL_SOCKET_BUFFER_SIZE];
-   struct nlmsghdr *nlh;
-
-   nlh = nftnl_nlmsg_build_hdr(buf, NFT_MSG_DELRULE,
-   nftnl_rule_get_u32(nlr, NFTNL_RULE_FAMILY),
-   NLM_F_ACK, seq);
-   nftnl_rule_nlmsg_build_payload(nlh, nlr);
-
-   return nft_mnl_talk(nf_sock, nlh, nlh->nlmsg_len, NULL, NULL);
-}
-
 static int rule_cb(const struct nlmsghdr *nlh, void *data)
 {
struct nftnl_rule_list *nlr_list = data;
-- 
2.13.1

--
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: [nft PATCH 2/6] monitor: Fix printing of set declarations

2017-07-26 Thread Phil Sutter 
On Wed, Jul 26, 2017 at 01:18:03PM +0200, Pablo Neira Ayuso wrote:
> On Tue, Jul 25, 2017 at 07:09:34PM +0200, Phil Sutter wrote:
> > On Tue, Jul 25, 2017 at 05:57:41PM +0200, Pablo Neira Ayuso wrote:
> > > On Tue, Jul 25, 2017 at 04:56:25PM +0200, Phil Sutter wrote:
> > > > diff --git a/tests/monitor/testcases/set-maps.t 
> > > > b/tests/monitor/testcases/set-maps.t
> > > > index d94016beb0767..6ea36cb9d11d6 100644
> > > > --- a/tests/monitor/testcases/set-maps.t
> > > > +++ b/tests/monitor/testcases/set-maps.t
> > > > @@ -2,7 +2,7 @@
> > > >  I add table ip t
> > > >  O add table ip t
> > > >  I add map ip t portip { type inet_service: ipv4_addr; flags interval; }
> > > > -O add map ip t portip { type inet_service : ipv4_addr;flags interval }
> > > > +O add map ip t portip { type inet_service : ipv4_addr;flags interval; }
> > > 
> > > So the proposal is to remove the whitespace? I think it's more
> > > readable the way it is already.
> > 
> > No, it is about the missing semicolon after 'flags interval' (and after
> > 'timeout' and 'gc-interval' options if present). The output of 'nft
> > monitor' is not syntactically correct without this.
> > 
> > You are correct in that it removes the whitespace before the closing
> > brace, but patch 4 "fixes" that by making 'stmt_separator' consist of
> > semicolon + whitespace.
> > 
> > Is it clear now?
> 
> Thanks for explaining, yes this clarifies. Please, add examples next
> time to your commit messages.

Yes, will do. And in that case I already did, please see v2 containing
the remaining patches (starting with message ID
20170725183944.9377-1-p...@nwl.cc).

> BTW, no need to update .t files after patch 4/6 to add the space after
> semicolon?

No, because I changed diff options along with the fix for NEWGEN message
to ignore all whitespace, not just at EOL.

Thanks, Phil
--
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 v2 nf-next] netfilter: nft_set: use sequence count to avoid read lock in most cases

2017-07-26 Thread Florian Westphal 
switch to lockless lockup. write side now also increments sequence
counter.  On lookup, sample counter value and only take the lock
if we did not find a match and the counter has changed.

This avoids need to write to private area in normal (lookup) cases.

In case we detect a writer (seqretry is true) we fall back to taking
the readlock.

The readlock is also used during dumps to ensure we get a consistent
tree walk.

Similar technique (rbtree+seqlock) was used by David Howells in rxrpc.

v2: break lookup loop in case the sequence count changed (Eric)

Suggested-by: Eric Dumazet 
Signed-off-by: Florian Westphal 
---
 net/netfilter/nft_set_rbtree.c | 47 --
 1 file changed, 36 insertions(+), 11 deletions(-)

diff --git a/net/netfilter/nft_set_rbtree.c b/net/netfilter/nft_set_rbtree.c
index bce5382f1d49..ff968d96b823 100644
--- a/net/netfilter/nft_set_rbtree.c
+++ b/net/netfilter/nft_set_rbtree.c
@@ -19,8 +19,9 @@
 #include 
 
 struct nft_rbtree {
-   rwlock_tlock;
struct rb_root  root;
+   rwlock_tlock;
+   seqcount_t  count;
 };
 
 struct nft_rbtree_elem {
@@ -40,8 +41,9 @@ static bool nft_rbtree_equal(const struct nft_set *set, const 
void *this,
return memcmp(this, nft_set_ext_key(&interval->ext), set->klen) == 0;
 }
 
-static bool nft_rbtree_lookup(const struct net *net, const struct nft_set *set,
- const u32 *key, const struct nft_set_ext **ext)
+static bool __nft_rbtree_lookup(const struct net *net, const struct nft_set 
*set,
+   const u32 *key, const struct nft_set_ext **ext,
+   unsigned int seq)
 {
struct nft_rbtree *priv = nft_set_priv(set);
const struct nft_rbtree_elem *rbe, *interval = NULL;
@@ -50,15 +52,17 @@ static bool nft_rbtree_lookup(const struct net *net, const 
struct nft_set *set,
const void *this;
int d;
 
-   read_lock_bh(&priv->lock);
-   parent = priv->root.rb_node;
+   parent = rcu_dereference_raw(priv->root.rb_node);
while (parent != NULL) {
+   if (read_seqcount_retry(&priv->count, seq))
+   return false;
+
rbe = rb_entry(parent, struct nft_rbtree_elem, node);
 
this = nft_set_ext_key(&rbe->ext);
d = memcmp(this, key, set->klen);
if (d < 0) {
-   parent = parent->rb_left;
+   parent = rcu_dereference_raw(parent->rb_left);
if (interval &&
nft_rbtree_equal(set, this, interval) &&
nft_rbtree_interval_end(this) &&
@@ -66,15 +70,14 @@ static bool nft_rbtree_lookup(const struct net *net, const 
struct nft_set *set,
continue;
interval = rbe;
} else if (d > 0)
-   parent = parent->rb_right;
+   parent = rcu_dereference_raw(parent->rb_right);
else {
if (!nft_set_elem_active(&rbe->ext, genmask)) {
-   parent = parent->rb_left;
+   parent = rcu_dereference_raw(parent->rb_left);
continue;
}
if (nft_rbtree_interval_end(rbe))
goto out;
-   read_unlock_bh(&priv->lock);
 
*ext = &rbe->ext;
return true;
@@ -84,15 +87,32 @@ static bool nft_rbtree_lookup(const struct net *net, const 
struct nft_set *set,
if (set->flags & NFT_SET_INTERVAL && interval != NULL &&
nft_set_elem_active(&interval->ext, genmask) &&
!nft_rbtree_interval_end(interval)) {
-   read_unlock_bh(&priv->lock);
*ext = &interval->ext;
return true;
}
 out:
-   read_unlock_bh(&priv->lock);
return false;
 }
 
+static bool nft_rbtree_lookup(const struct net *net, const struct nft_set *set,
+ const u32 *key, const struct nft_set_ext **ext)
+{
+   struct nft_rbtree *priv = nft_set_priv(set);
+   unsigned int seq = read_seqcount_begin(&priv->count);
+   bool ret;
+
+   ret = __nft_rbtree_lookup(net, set, key, ext, seq);
+   if (ret || !read_seqcount_retry(&priv->count, seq))
+   return ret;
+
+   read_lock_bh(&priv->lock);
+   seq = read_seqcount_begin(&priv->count);
+   ret = __nft_rbtree_lookup(net, set, key, ext, seq);
+   read_unlock_bh(&priv->lock);
+
+   return ret;
+}
+
 static int __nft_rbtree_insert(const struct net *net, const struct nft_set 
*set,
   struct nft_rbtree_elem *new,
   struct nft_set_ext **ext)
@@ -144,7 +164,9 @@ static int nft_rbtree_insert(c

Re: [PATCH nf-next] netfilter: nft_set_rbtree: use seqcount to avoid lock in most cases

2017-07-26 Thread Pablo Neira Ayuso 
On Wed, Jul 26, 2017 at 01:15:06PM +0200, Pablo Neira Ayuso wrote:
> On Wed, Jul 26, 2017 at 02:09:41AM +0200, Florian Westphal wrote:
> [...]
> > @@ -144,7 +159,9 @@ static int nft_rbtree_insert(const struct net *net, 
> > const struct nft_set *set,
> > int err;
> >  
> > write_lock_bh(&priv->lock);
> > +   write_seqcount_begin(&priv->count);
> > err = __nft_rbtree_insert(net, set, rbe, ext);
> > +   write_seqcount_end(&priv->count);
> > write_unlock_bh(&priv->lock);
> >  
> > return err;
> > @@ -158,7 +175,9 @@ static void nft_rbtree_remove(const struct net *net,
> > struct nft_rbtree_elem *rbe = elem->priv;
> >  
> > write_lock_bh(&priv->lock);
> 
> Do we need the spinlock anymore? This is protected by mutex from
> userspace, and we have no support for neither timeouts nor dynamic set
> population from packet path yet.

Forget this, I overlook we have a slow path in case seq mismatches.
--
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: [nft PATCH 2/6] monitor: Fix printing of set declarations

2017-07-26 Thread Pablo Neira Ayuso 
On Tue, Jul 25, 2017 at 07:09:34PM +0200, Phil Sutter wrote:
> On Tue, Jul 25, 2017 at 05:57:41PM +0200, Pablo Neira Ayuso wrote:
> > On Tue, Jul 25, 2017 at 04:56:25PM +0200, Phil Sutter wrote:
> > > diff --git a/tests/monitor/testcases/set-maps.t 
> > > b/tests/monitor/testcases/set-maps.t
> > > index d94016beb0767..6ea36cb9d11d6 100644
> > > --- a/tests/monitor/testcases/set-maps.t
> > > +++ b/tests/monitor/testcases/set-maps.t
> > > @@ -2,7 +2,7 @@
> > >  I add table ip t
> > >  O add table ip t
> > >  I add map ip t portip { type inet_service: ipv4_addr; flags interval; }
> > > -O add map ip t portip { type inet_service : ipv4_addr;flags interval }
> > > +O add map ip t portip { type inet_service : ipv4_addr;flags interval; }
> > 
> > So the proposal is to remove the whitespace? I think it's more
> > readable the way it is already.
> 
> No, it is about the missing semicolon after 'flags interval' (and after
> 'timeout' and 'gc-interval' options if present). The output of 'nft
> monitor' is not syntactically correct without this.
> 
> You are correct in that it removes the whitespace before the closing
> brace, but patch 4 "fixes" that by making 'stmt_separator' consist of
> semicolon + whitespace.
> 
> Is it clear now?

Thanks for explaining, yes this clarifies. Please, add examples next
time to your commit messages.

BTW, no need to update .t files after patch 4/6 to add the space after
semicolon?

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

Re: [PATCH nf-next] netfilter: nft_set_rbtree: use seqcount to avoid lock in most cases

2017-07-26 Thread Pablo Neira Ayuso 
On Wed, Jul 26, 2017 at 02:09:41AM +0200, Florian Westphal wrote:
[...]
> @@ -144,7 +159,9 @@ static int nft_rbtree_insert(const struct net *net, const 
> struct nft_set *set,
>   int err;
>  
>   write_lock_bh(&priv->lock);
> + write_seqcount_begin(&priv->count);
>   err = __nft_rbtree_insert(net, set, rbe, ext);
> + write_seqcount_end(&priv->count);
>   write_unlock_bh(&priv->lock);
>  
>   return err;
> @@ -158,7 +175,9 @@ static void nft_rbtree_remove(const struct net *net,
>   struct nft_rbtree_elem *rbe = elem->priv;
>  
>   write_lock_bh(&priv->lock);

Do we need the spinlock anymore? This is protected by mutex from
userspace, and we have no support for neither timeouts nor dynamic set
population from packet path yet.

> + write_seqcount_begin(&priv->count);
>   rb_erase(&rbe->node, &priv->root);
> + write_seqcount_end(&priv->count);
>   write_unlock_bh(&priv->lock);
>  }
>  
--
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 nf-next] netfilter: nft_set_rbtree: use seqcount to avoid lock in most cases

2017-07-26 Thread Florian Westphal 
Eric Dumazet  wrote:
> On Wed, 2017-07-26 at 02:09 +0200, Florian Westphal wrote:
> > switch to lockless lockup. write side now also increments sequence
> > counter.  On lookup, sample counter value and only take the lock
> > if we did not find a match and the counter has changed.
> > 
> > This avoids need to write to private area in normal (lookup) cases.
> > 
> > Note that we take the non-blocking variant (raw_seqcount_begin), i.e.
> > read side will not wait for writer to finish.
> > 
> > If we did not find a result we will fall back to use of read-lock.
> > 
> > The readlock is also used during dumps to ensure we get a consistent
> > tree walk.
> > 
> > Similar technique (rbtree+seqlock) was used by David Howells in rxrpc.
> 
> Please note that in commit b145425f269a17ed344d737f746b844dfac60c82
> ("inetpeer: remove AVL implementation in favor of RB tree")
> 
> I chose to also pass the sequence so that the lookup could abort.
> I am not sure that during rb tree write operations, some nodes could be
> left with some kind of loop.

I see.

Ok, I will spin a v2 and will pass the sequence too, thanks Eric.

If we have to abort on seqretry anyway then I can also use
read_seqcount_begin to force readers to wait until writer is done, so I
will change that as well.
--
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 V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

2017-07-26 Thread Pablo Neira Ayuso 
On Wed, Jul 26, 2017 at 11:27:16AM +0200, Florian Westphal wrote:
> Taehee Yoo  wrote:
> > If verdict is NF_STOLEN in the SYNPROXY target,
> > the skb is consumed.
> > However, ipt_do_table() always tries to get ip header from the skb.
> > So that, KASAN triggers the use-after-free message.
> 
> In case anyone wonders, ip6tables doesn't have this problem
> because we pass *skb, not ip6hdr to ip6_packet_match().

I think it would be good to make these code converge to what ip6tables
is doing while fixing up this?

> arptables has the same bug, it seems (no target returns STOLEN,
> but I think we should fix it there as well).

Yes, even if no target returns what triggers the problem, it's good if
we fix this now so we make sure whatever new extension gets in in the
future works accordingly.

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

Re: [PATCH nf-next] netfilter: nft_set_rbtree: use seqcount to avoid lock in most cases

2017-07-26 Thread Eric Dumazet 
On Wed, 2017-07-26 at 02:09 +0200, Florian Westphal wrote:
> switch to lockless lockup. write side now also increments sequence
> counter.  On lookup, sample counter value and only take the lock
> if we did not find a match and the counter has changed.
> 
> This avoids need to write to private area in normal (lookup) cases.
> 
> Note that we take the non-blocking variant (raw_seqcount_begin), i.e.
> read side will not wait for writer to finish.
> 
> If we did not find a result we will fall back to use of read-lock.
> 
> The readlock is also used during dumps to ensure we get a consistent
> tree walk.
> 
> Similar technique (rbtree+seqlock) was used by David Howells in rxrpc.


Please note that in commit b145425f269a17ed344d737f746b844dfac60c82
("inetpeer: remove AVL implementation in favor of RB tree")

I chose to also pass the sequence so that the lookup could abort.

I am not sure that during rb tree write operations, some nodes could be
left with some kind of loop.



--
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 nf-next] netfilter: nf_hook_ops structs can be const

2017-07-26 Thread Florian Westphal 
We no longer place these on a list so they can be const.

Signed-off-by: Florian Westphal 
---
 drivers/net/ipvlan/ipvlan_main.c   | 2 +-
 net/bridge/br_netfilter_hooks.c| 2 +-
 net/bridge/netfilter/ebtable_filter.c  | 2 +-
 net/bridge/netfilter/ebtable_nat.c | 2 +-
 net/decnet/netfilter/dn_rtmsg.c| 2 +-
 net/ipv4/netfilter/ipt_CLUSTERIP.c | 2 +-
 net/ipv4/netfilter/ipt_SYNPROXY.c  | 2 +-
 net/ipv4/netfilter/iptable_nat.c   | 2 +-
 net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c | 2 +-
 net/ipv4/netfilter/nf_defrag_ipv4.c| 2 +-
 net/ipv6/ila/ila_xlat.c| 2 +-
 net/ipv6/netfilter/ip6t_SYNPROXY.c | 2 +-
 net/ipv6/netfilter/ip6table_nat.c  | 2 +-
 net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c | 2 +-
 net/ipv6/netfilter/nf_defrag_ipv6_hooks.c  | 2 +-
 net/netfilter/ipvs/ip_vs_core.c| 2 +-
 security/selinux/hooks.c   | 2 +-
 security/smack/smack_netfilter.c   | 2 +-
 18 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index fdde20735416..943e6907dc19 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -15,7 +15,7 @@ struct ipvlan_netns {
unsigned int ipvl_nf_hook_refcnt;
 };
 
-static struct nf_hook_ops ipvl_nfops[] __read_mostly = {
+static const struct nf_hook_ops ipvl_nfops[] = {
{
.hook = ipvlan_nf_input,
.pf   = NFPROTO_IPV4,
diff --git a/net/bridge/br_netfilter_hooks.c b/net/bridge/br_netfilter_hooks.c
index 2261e5194c82..626f4b2cef16 100644
--- a/net/bridge/br_netfilter_hooks.c
+++ b/net/bridge/br_netfilter_hooks.c
@@ -887,7 +887,7 @@ EXPORT_SYMBOL_GPL(br_netfilter_enable);
 
 /* For br_nf_post_routing, we need (prio = NF_BR_PRI_LAST), because
  * br_dev_queue_push_xmit is called afterwards */
-static struct nf_hook_ops br_nf_ops[] __read_mostly = {
+static const struct nf_hook_ops br_nf_ops[] = {
{
.hook = br_nf_pre_routing,
.pf = NFPROTO_BRIDGE,
diff --git a/net/bridge/netfilter/ebtable_filter.c 
b/net/bridge/netfilter/ebtable_filter.c
index f22ef7c21913..45a00dbdbcad 100644
--- a/net/bridge/netfilter/ebtable_filter.c
+++ b/net/bridge/netfilter/ebtable_filter.c
@@ -70,7 +70,7 @@ ebt_out_hook(void *priv, struct sk_buff *skb,
return ebt_do_table(skb, state, state->net->xt.frame_filter);
 }
 
-static struct nf_hook_ops ebt_ops_filter[] __read_mostly = {
+static const struct nf_hook_ops ebt_ops_filter[] = {
{
.hook   = ebt_in_hook,
.pf = NFPROTO_BRIDGE,
diff --git a/net/bridge/netfilter/ebtable_nat.c 
b/net/bridge/netfilter/ebtable_nat.c
index 2f7a4f314406..4ecf50662b7d 100644
--- a/net/bridge/netfilter/ebtable_nat.c
+++ b/net/bridge/netfilter/ebtable_nat.c
@@ -70,7 +70,7 @@ ebt_nat_out(void *priv, struct sk_buff *skb,
return ebt_do_table(skb, state, state->net->xt.frame_nat);
 }
 
-static struct nf_hook_ops ebt_ops_nat[] __read_mostly = {
+static const struct nf_hook_ops ebt_ops_nat[] = {
{
.hook   = ebt_nat_out,
.pf = NFPROTO_BRIDGE,
diff --git a/net/decnet/netfilter/dn_rtmsg.c b/net/decnet/netfilter/dn_rtmsg.c
index aa8ffecc46a4..ab395e55cd78 100644
--- a/net/decnet/netfilter/dn_rtmsg.c
+++ b/net/decnet/netfilter/dn_rtmsg.c
@@ -115,7 +115,7 @@ static inline void dnrmg_receive_user_skb(struct sk_buff 
*skb)
RCV_SKB_FAIL(-EINVAL);
 }
 
-static struct nf_hook_ops dnrmg_ops __read_mostly = {
+static const struct nf_hook_ops dnrmg_ops = {
.hook   = dnrmg_hook,
.pf = NFPROTO_DECNET,
.hooknum= NF_DN_ROUTE,
diff --git a/net/ipv4/netfilter/ipt_CLUSTERIP.c 
b/net/ipv4/netfilter/ipt_CLUSTERIP.c
index 7d72decb80f9..6637e8b37ee2 100644
--- a/net/ipv4/netfilter/ipt_CLUSTERIP.c
+++ b/net/ipv4/netfilter/ipt_CLUSTERIP.c
@@ -624,7 +624,7 @@ arp_mangle(void *priv,
return NF_ACCEPT;
 }
 
-static struct nf_hook_ops cip_arp_ops __read_mostly = {
+static const struct nf_hook_ops cip_arp_ops = {
.hook = arp_mangle,
.pf = NFPROTO_ARP,
.hooknum = NF_ARP_OUT,
diff --git a/net/ipv4/netfilter/ipt_SYNPROXY.c 
b/net/ipv4/netfilter/ipt_SYNPROXY.c
index f1528f7175a8..811689e523c3 100644
--- a/net/ipv4/netfilter/ipt_SYNPROXY.c
+++ b/net/ipv4/netfilter/ipt_SYNPROXY.c
@@ -416,7 +416,7 @@ static unsigned int ipv4_synproxy_hook(void *priv,
return NF_ACCEPT;
 }
 
-static struct nf_hook_ops ipv4_synproxy_ops[] __read_mostly = {
+static const struct nf_hook_ops ipv4_synproxy_ops[] = {
{
.hook   = ipv4_synproxy_hook,
.pf = NFPROTO_IPV4,
diff --git a/net/ipv4/netfilter/iptable_nat.c b/net/ipv4/netfilter/iptable_nat.c
index 138a24bc76ad..a

Re: [PATCH V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

2017-07-26 Thread Florian Westphal 
Taehee Yoo  wrote:
> If verdict is NF_STOLEN in the SYNPROXY target,
> the skb is consumed.
> However, ipt_do_table() always tries to get ip header from the skb.
> So that, KASAN triggers the use-after-free message.

In case anyone wonders, ip6tables doesn't have this problem
because we pass *skb, not ip6hdr to ip6_packet_match().

arptables has the same bug, it seems (no target returns STOLEN,
but I think we should fix it there as well).

--
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 V2] netfilter: x_tables: Fix use-after-free in ipt_do_table.

2017-07-26 Thread Taehee Yoo 
If verdict is NF_STOLEN in the SYNPROXY target,
the skb is consumed.
However, ipt_do_table() always tries to get ip header from the skb.
So that, KASAN triggers the use-after-free message.

We can reproduce this message using below command.
  # iptables -I INPUT -p tcp -j SYNPROXY --mss 1460

[ 193.542265] BUG: KASAN: use-after-free in ipt_do_table+0x1405/0x1c10
[ ... ]
[ 193.578603] Call Trace:
[ 193.581590] 
[ 193.584107] dump_stack+0x68/0xa0
[ 193.588168] print_address_description+0x78/0x290
[ 193.593828] ? ipt_do_table+0x1405/0x1c10
[ 193.598690] kasan_report+0x230/0x340
[ 193.603194] __asan_report_load2_noabort+0x19/0x20
[ 193.608950] ipt_do_table+0x1405/0x1c10
[ 193.613591] ? rcu_read_lock_held+0xae/0xd0
[ 193.618631] ? ip_route_input_rcu+0x27d7/0x4270
[ 193.624348] ? ipt_do_table+0xb68/0x1c10
[ 193.629124] ? do_add_counters+0x620/0x620
[ 193.634234] ? iptable_filter_net_init+0x60/0x60
[ ... ]

After this patch, only when verdict is XT_CONTINUE,
ipt_do_table() tries to get ip header.

Signed-off-by: Taehee Yoo 
---
V2:
 - Change commit log message.

V1:
 - Initial Version

 net/ipv4/netfilter/ip_tables.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
index 2a55a40..622ed28 100644
--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -352,13 +352,14 @@ ipt_do_table(struct sk_buff *skb,
acpar.targinfo = t->data;
 
verdict = t->u.kernel.target->target(skb, &acpar);
-   /* Target might have changed stuff. */
-   ip = ip_hdr(skb);
-   if (verdict == XT_CONTINUE)
+   if (verdict == XT_CONTINUE) {
+   /* Target might have changed stuff. */
+   ip = ip_hdr(skb);
e = ipt_next_entry(e);
-   else
+   } else {
/* Verdict */
break;
+   }
} while (!acpar.hotdrop);
 
xt_write_recseq_end(addend);
-- 
2.9.3

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