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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 20 Jun 2022 11:28:57 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     chenzhen 00642392 <chenzhen126@...wei.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>,
        "Chenxiang (EulerOS)" <rose.chen@...wei.com>
Subject: Re: [Patch net] net_sched: cls_route: free the old filter only when
 it has been removed

On Fri, Jun 17, 2022 at 11:20 PM chenzhen 00642392
<chenzhen126@...wei.com> wrote:
>
> From: Zhen Chen <chenzhen126@...wei.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]);