[net] net_sched: cls_route: free the old filter only when it has been removed

Commit Message

Zhen Chen June 18, 2022, 6:18 a.m. UTC

From: Zhen Chen <[e-mail]>

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.

Fix this by putting the free logic together with the remove action.

Reported-and-tested-by: [e-mail]
Fixes: 1109c00547fc ("net: sched: RCU cls_route")
Signed-off-by: Zhen Chen <[e-mail]>
---
 net/sched/cls_route.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Comments

Cong Wang June 20, 2022, 6:28 p.m. UTC | #1

On Fri, Jun 17, 2022 at 11:20 PM chenzhen 00642392
<[e-mail]> wrote:
>
> From: Zhen Chen <[e-mail]>
>
> 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]);

Patch

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: