[PATCH] flowtop: Fix use-after-free on filter reload

[Vadim Kochan]

There is missing logic which removes flown entry from
related proc's entry while destroying global flows list on
filter reloading, hence add common __flow_list_del_entry which
handles this logic for both cases - when ct destroyed or filter
changed.

This is a 2nd fix for issue #183.

Signed-off-by: Vadim Kochan <vad...@gmail.com>
---
flowtop.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)

diff --git a/flowtop.c b/flowtop.c
index 8b69d65..7de4d11 100644
--- a/flowtop.c
+++ b/flowtop.c
@@ -470,20 +470,24 @@ static struct flow_entry *flow_list_find_id(struct flow_list *fl, uint32_t id)
return NULL;
}

+static void __flow_list_del_entry(struct flow_list *fl, struct flow_entry *n)
+{
+ if (n->proc) {
+ cds_list_del_rcu(&n->proc_head);
+ n->proc->flows_count--;
+ }
+
+ cds_list_del_rcu(&n->entry);
+ call_rcu(&n->rcu, flow_entry_xfree_rcu);
+}
+
static int flow_list_del_entry(struct flow_list *fl, const struct nf_conntrack *ct)
{
struct flow_entry *n;

n = flow_list_find_id(fl, nfct_get_attr_u32(ct, ATTR_ID));
- if (n) {
- if (n->proc) {
- cds_list_del_rcu(&n->proc_head);
- n->proc->flows_count--;
- }
-
- cds_list_del_rcu(&n->entry);
- call_rcu(&n->rcu, flow_entry_xfree_rcu);
- }
+ if (n)
+ __flow_list_del_entry(fl, n);

return NFCT_CB_CONTINUE;
}
@@ -492,10 +496,8 @@ static void flow_list_destroy(struct flow_list *fl)
{
struct flow_entry *n, *tmp;

- cds_list_for_each_entry_safe(n, tmp, &fl->head, entry) {
- cds_list_del_rcu(&n->entry);
- call_rcu(&n->rcu, flow_entry_xfree_rcu);
- }
+ cds_list_for_each_entry_safe(n, tmp, &fl->head, entry)
+ __flow_list_del_entry(fl, n);
}

static void proc_list_init(struct proc_list *proc_list)
@@ -562,7 +564,7 @@ static void flow_entry_find_process(struct flow_entry *n)
p->stat.bytes_dst += n->stat.bytes_dst;
p->flows_count++;

- cds_list_add(&n->proc_head, &p->flows);
+ cds_list_add_rcu(&n->proc_head, &p->flows);
n->proc = p;
}

--
2.14.1

[Tobias Klauser]

On 2017-12-18 at 23:38:18 +0100, Vadim Kochan <vad...@gmail.com> wrote:
> There is missing logic which removes flown entry from
> related proc's entry while destroying global flows list on
> filter reloading, hence add common __flow_list_del_entry which
> handles this logic for both cases - when ct destroyed or filter
> changed.
>
> This is a 2nd fix for issue #183.

Thanks for the patch. While it is certainly correct, it unfortunately
still doesn't fix #183 properly. I can still trigger a segfault by
repeatedly enabling/disabling TCP, UDP and ICMP flows ('T', 'U' or 'I'
key).

[Vadim Kochan]

Thats really strange, because before this patch I really easy triggered the issue, but

now I cant.

[Vadim Kochan]

May it possible that you tried flowtop compiled without the fix ?

[Tobias Klauser]

On 2017-12-19 at 11:24:40 +0100, Vadim Kochan <vad...@gmail.com> wrote:
> May it possible that you tried flowtop compiled without the fix ?

No, I made sure to have the patch applied and recompiled flowtop. I can
still quite reliably reproduce the issue and flowtop sometimes even
segfaults on startup before displaying anything.

[Vadim Kochan]

Hi Tobias,

Looks like https://github.com/netsniff-ng/netsniff-ng/issues/183

is not reproducible, do you still see issues with flowtop ?

Regards,

Vadim Kochan

[Tobias Klauser]

The issue is still reproducible for me on latest master. Unfortunately
not in gdb though.

But since the bug does appear to occur less often (at least you and the
original reporter can no longer reproduce), I think we should still go
ahead with the release.

Thanks
Tobias