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

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

[chenzhen 00642392]

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.

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

Reported-and-tested-by: syzbot+2e3efb5eb71cb5075ba7@syzkaller.appspotmail.com
Fixes: 1109c00547fc ("net: sched: RCU cls_route")
Signed-off-by: Zhen Chen <chenzhen126@huawei.com>
---
 net/sched/cls_route.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

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

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

[Cong Wang]

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]);