* [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects
@ 2016-08-22 13:58 Liping Zhang
2016-08-22 13:58 ` [PATCH nf 2/3] netfilter: cttimeout: put back l4proto when replacing timeout policy Liping Zhang
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Liping Zhang @ 2016-08-22 13:58 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
cttimeout and acct objects are deleted from the list while traversing
it, so use list_for_each_entry is unsafe here.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nfnetlink_acct.c | 6 +++---
net/netfilter/nfnetlink_cttimeout.c | 5 +++--
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
index 70eb2f6a..d44d89b 100644
--- a/net/netfilter/nfnetlink_acct.c
+++ b/net/netfilter/nfnetlink_acct.c
@@ -343,12 +343,12 @@ static int nfnl_acct_del(struct net *net, struct sock *nfnl,
struct sk_buff *skb, const struct nlmsghdr *nlh,
const struct nlattr * const tb[])
{
- char *acct_name;
- struct nf_acct *cur;
+ struct nf_acct *cur, *tmp;
int ret = -ENOENT;
+ char *acct_name;
if (!tb[NFACCT_NAME]) {
- list_for_each_entry(cur, &net->nfnl_acct_list, head)
+ list_for_each_entry_safe(cur, tmp, &net->nfnl_acct_list, head)
nfnl_acct_try_del(cur);
return 0;
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 68216cd..f74fee1 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -350,12 +350,13 @@ static int cttimeout_del_timeout(struct net *net, struct sock *ctnl,
const struct nlmsghdr *nlh,
const struct nlattr * const cda[])
{
- struct ctnl_timeout *cur;
+ struct ctnl_timeout *cur, *tmp;
int ret = -ENOENT;
char *name;
if (!cda[CTA_TIMEOUT_NAME]) {
- list_for_each_entry(cur, &net->nfct_timeout_list, head)
+ list_for_each_entry_safe(cur, tmp, &net->nfct_timeout_list,
+ head)
ctnl_timeout_try_del(net, cur);
return 0;
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH nf 2/3] netfilter: cttimeout: put back l4proto when replacing timeout policy
2016-08-22 13:58 [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Liping Zhang
@ 2016-08-22 13:58 ` Liping Zhang
2016-08-25 10:57 ` Pablo Neira Ayuso
2016-08-22 13:58 ` [PATCH nf 3/3] netfilter: cttimeout: unlink timeout objs in the unconfirmed ct lists Liping Zhang
2016-08-25 10:57 ` [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Pablo Neira Ayuso
2 siblings, 1 reply; 6+ messages in thread
From: Liping Zhang @ 2016-08-22 13:58 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
We forget to call nf_ct_l4proto_put when replacing the existing
timeout policy. Acctually, there's no need to get ct l4proto
before doing replace, so we can move it to a later position.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nfnetlink_cttimeout.c | 33 +++++++++++++++------------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index f74fee1..6844c7a 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -98,31 +98,28 @@ static int cttimeout_new_timeout(struct net *net, struct sock *ctnl,
break;
}
- l4proto = nf_ct_l4proto_find_get(l3num, l4num);
-
- /* This protocol is not supportted, skip. */
- if (l4proto->l4proto != l4num) {
- ret = -EOPNOTSUPP;
- goto err_proto_put;
- }
-
if (matching) {
if (nlh->nlmsg_flags & NLM_F_REPLACE) {
/* You cannot replace one timeout policy by another of
* different kind, sorry.
*/
if (matching->l3num != l3num ||
- matching->l4proto->l4proto != l4num) {
- ret = -EINVAL;
- goto err_proto_put;
- }
-
- ret = ctnl_timeout_parse_policy(&matching->data,
- l4proto, net,
- cda[CTA_TIMEOUT_DATA]);
- return ret;
+ matching->l4proto->l4proto != l4num)
+ return -EINVAL;
+
+ return ctnl_timeout_parse_policy(&matching->data,
+ matching->l4proto, net,
+ cda[CTA_TIMEOUT_DATA]);
}
- ret = -EBUSY;
+
+ return -EBUSY;
+ }
+
+ l4proto = nf_ct_l4proto_find_get(l3num, l4num);
+
+ /* This protocol is not supportted, skip. */
+ if (l4proto->l4proto != l4num) {
+ ret = -EOPNOTSUPP;
goto err_proto_put;
}
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH nf 3/3] netfilter: cttimeout: unlink timeout objs in the unconfirmed ct lists
2016-08-22 13:58 [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Liping Zhang
2016-08-22 13:58 ` [PATCH nf 2/3] netfilter: cttimeout: put back l4proto when replacing timeout policy Liping Zhang
@ 2016-08-22 13:58 ` Liping Zhang
2016-08-25 10:58 ` Pablo Neira Ayuso
2016-08-25 10:57 ` [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Pablo Neira Ayuso
2 siblings, 1 reply; 6+ messages in thread
From: Liping Zhang @ 2016-08-22 13:58 UTC (permalink / raw)
To: pablo; +Cc: netfilter-devel, Liping Zhang
From: Liping Zhang <liping.zhang@spreadtrum.com>
KASAN reported this bug:
BUG: KASAN: use-after-free in icmp_packet+0x25/0x50 [nf_conntrack_ipv4] at
addr ffff880002db08c8
Read of size 4 by task lt-nf-queue/19041
Call Trace:
<IRQ> [<ffffffff815eeebb>] dump_stack+0x63/0x88
[<ffffffff813386f8>] kasan_report_error+0x528/0x560
[<ffffffff81338cc8>] kasan_report+0x58/0x60
[<ffffffffa07393f5>] ? icmp_packet+0x25/0x50 [nf_conntrack_ipv4]
[<ffffffff81337551>] __asan_load4+0x61/0x80
[<ffffffffa07393f5>] icmp_packet+0x25/0x50 [nf_conntrack_ipv4]
[<ffffffffa06ecaa0>] nf_conntrack_in+0x550/0x980 [nf_conntrack]
[<ffffffffa06ec550>] ? __nf_conntrack_confirm+0xb10/0xb10 [nf_conntrack]
[ ... ]
The main reason is that we missed to unlink the timeout objects in the
unconfirmed ct lists, so we will access the timeout objects that have
already been freed.
Signed-off-by: Liping Zhang <liping.zhang@spreadtrum.com>
---
net/netfilter/nfnetlink_cttimeout.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/net/netfilter/nfnetlink_cttimeout.c b/net/netfilter/nfnetlink_cttimeout.c
index 6844c7a..139e086 100644
--- a/net/netfilter/nfnetlink_cttimeout.c
+++ b/net/netfilter/nfnetlink_cttimeout.c
@@ -302,7 +302,16 @@ static void ctnl_untimeout(struct net *net, struct ctnl_timeout *timeout)
const struct hlist_nulls_node *nn;
unsigned int last_hsize;
spinlock_t *lock;
- int i;
+ int i, cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct ct_pcpu *pcpu = per_cpu_ptr(net->ct.pcpu_lists, cpu);
+
+ spin_lock_bh(&pcpu->lock);
+ hlist_nulls_for_each_entry(h, nn, &pcpu->unconfirmed, hnnode)
+ untimeout(h, timeout);
+ spin_unlock_bh(&pcpu->lock);
+ }
local_bh_disable();
restart:
--
2.5.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH nf 3/3] netfilter: cttimeout: unlink timeout objs in the unconfirmed ct lists
2016-08-22 13:58 ` [PATCH nf 3/3] netfilter: cttimeout: unlink timeout objs in the unconfirmed ct lists Liping Zhang
@ 2016-08-25 10:58 ` Pablo Neira Ayuso
0 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-25 10:58 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Mon, Aug 22, 2016 at 09:58:18PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
>
> KASAN reported this bug:
> BUG: KASAN: use-after-free in icmp_packet+0x25/0x50 [nf_conntrack_ipv4] at
> addr ffff880002db08c8
> Read of size 4 by task lt-nf-queue/19041
> Call Trace:
> <IRQ> [<ffffffff815eeebb>] dump_stack+0x63/0x88
> [<ffffffff813386f8>] kasan_report_error+0x528/0x560
> [<ffffffff81338cc8>] kasan_report+0x58/0x60
> [<ffffffffa07393f5>] ? icmp_packet+0x25/0x50 [nf_conntrack_ipv4]
> [<ffffffff81337551>] __asan_load4+0x61/0x80
> [<ffffffffa07393f5>] icmp_packet+0x25/0x50 [nf_conntrack_ipv4]
> [<ffffffffa06ecaa0>] nf_conntrack_in+0x550/0x980 [nf_conntrack]
> [<ffffffffa06ec550>] ? __nf_conntrack_confirm+0xb10/0xb10 [nf_conntrack]
> [ ... ]
>
> The main reason is that we missed to unlink the timeout objects in the
> unconfirmed ct lists, so we will access the timeout objects that have
> already been freed.
Applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects
2016-08-22 13:58 [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Liping Zhang
2016-08-22 13:58 ` [PATCH nf 2/3] netfilter: cttimeout: put back l4proto when replacing timeout policy Liping Zhang
2016-08-22 13:58 ` [PATCH nf 3/3] netfilter: cttimeout: unlink timeout objs in the unconfirmed ct lists Liping Zhang
@ 2016-08-25 10:57 ` Pablo Neira Ayuso
2 siblings, 0 replies; 6+ messages in thread
From: Pablo Neira Ayuso @ 2016-08-25 10:57 UTC (permalink / raw)
To: Liping Zhang; +Cc: netfilter-devel, Liping Zhang
On Mon, Aug 22, 2016 at 09:58:16PM +0800, Liping Zhang wrote:
> From: Liping Zhang <liping.zhang@spreadtrum.com>
>
> cttimeout and acct objects are deleted from the list while traversing
> it, so use list_for_each_entry is unsafe here.
Also applied, thanks.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-08-25 10:59 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-22 13:58 [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Liping Zhang
2016-08-22 13:58 ` [PATCH nf 2/3] netfilter: cttimeout: put back l4proto when replacing timeout policy Liping Zhang
2016-08-25 10:57 ` Pablo Neira Ayuso
2016-08-22 13:58 ` [PATCH nf 3/3] netfilter: cttimeout: unlink timeout objs in the unconfirmed ct lists Liping Zhang
2016-08-25 10:58 ` Pablo Neira Ayuso
2016-08-25 10:57 ` [PATCH nf 1/3] netfilter: nfnetlink: use list_for_each_entry_safe to delete all objects Pablo Neira Ayuso
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).