Message ID | 20220618061840.1012529-1-chenzhen126@huawei.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | [net] net_sched: cls_route: free the old filter only when it has been removed | expand |
On Fri, Jun 17, 2022 at 11:20 PM chenzhen 00642392 <chenzhen126@huawei.com> wrote: > > From: Zhen Chen <chenzhen126@huawei.com> > > Syzbot reported a ODEBUG bug in route4_destroy(), it is actually a > use-after-free issue when route4_destroy() goes through the hashtable. > > The root cause is that after route4_change() inserts a new filter into the > hashtable and finds an old filter, it will not remove the old one from the > table if fold->handle is 0, but free the fold as the final step. This seems reasonable but see below. > > Fix this by putting the free logic together with the remove action. This does not look correct. You just move the deletion logic upper to a narrowed case. The if case you moved to also does the deletion without your patch, so I fail to see how this could solve the problem. If we just follow your logic here, should we have the following patch instead? But I am still not sure whether we need to treat the 0 handle special here. diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c index a35ab8c27866..758c21f9d628 100644 --- a/net/sched/cls_route.c +++ b/net/sched/cls_route.c @@ -526,7 +526,7 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, rcu_assign_pointer(f->next, f1); rcu_assign_pointer(*fp, f); - if (fold && fold->handle && f->handle != fold->handle) { + if (fold && f->handle != fold->handle) { th = to_hash(fold->handle); h = from_hash(fold->handle >> 16); b = rtnl_dereference(head->table[th]);
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c index a35ab8c27866..3917b84700b4 100644 --- a/net/sched/cls_route.c +++ b/net/sched/cls_route.c @@ -536,6 +536,9 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, fp = &pfp->next, pfp = rtnl_dereference(*fp)) { if (pfp == fold) { rcu_assign_pointer(*fp, fold->next); + tcf_unbind_filter(tp, &fold->res); + tcf_exts_get_net(&fold->exts); + tcf_queue_work(&fold->rwork, route4_delete_filter_work); break; } } @@ -544,11 +547,6 @@ static int route4_change(struct net *net, struct sk_buff *in_skb, route4_reset_fastmap(head); *arg = f; - if (fold) { - tcf_unbind_filter(tp, &fold->res); - tcf_exts_get_net(&fold->exts); - tcf_queue_work(&fold->rwork, route4_delete_filter_work); - } return 0; errout: