diff mbox series

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

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

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers fail 3 blamed authors not CCed: john.fastabend@gmail.com davem@davemloft.net edumazet@google.com; 5 maintainers not CCed: edumazet@google.com pabeni@redhat.com kuba@kernel.org davem@davemloft.net john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: line length of 96 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Zhen Chen June 18, 2022, 6:18 a.m. UTC
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(-)

Comments

Cong Wang June 20, 2022, 6:28 p.m. UTC | #1
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 mbox series

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: