| Message ID | 20221208032216.63513-1-chenzhongjin@huawei.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net/sched: Fix memory leak in tcindex_set_parms | expand |
Hi, On 2022/12/8 11:22, Chen Zhongjin wrote: > syzkaller reported a memleak: > https://syzkaller.appspot.com/bug?id=e061e6cd46417ee6566dc249d8f982c0b5977a52 > > unreferenced object 0xffff888107813900 (size 256): > backtrace: > kcalloc include/linux/slab.h:636 [inline] > tcf_exts_init include/net/pkt_cls.h:250 [inline] > tcindex_set_parms+0xa7/0xbe0 net/sched/cls_tcindex.c:342 > tcindex_change+0xdf/0x120 net/sched/cls_tcindex.c:553 > tc_new_tfilter+0x4f2/0x1100 net/sched/cls_api.c:2147 > ... > > The reproduce calls tc_new_tfilter() continuously: > > tc_new_tfilter()... > tcindex_set_parms() > tcf_exts_init(&e, ...) // alloc e->actions > tcf_exts_change(&r->exts, &e) > > tc_new_tfilter()... > tcindex_set_parms() > old_r = r // same as first r > tcindex_filter_result_init(old_r, cp, net); > // old_r is holding e->actions but here it calls memset(old_r, 0) > // so the previous e->actions is leaked > > So here tcf_exts_destroy() should be called to free old_r->exts.actions > before memset(old_r, 0) sets it to NULL. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Cc: stable@vger.kernel.org > Reported-by: syzbot+2f9183cb6f89b0e16586@syzkaller.appspotmail.com > Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com> > --- > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 355479c70a48 > --- > net/sched/cls_tcindex.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c > index 1c9eeb98d826..00a6c04a4b42 100644 > --- a/net/sched/cls_tcindex.c > +++ b/net/sched/cls_tcindex.c > @@ -479,6 +479,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, > } > > if (old_r && old_r != r) { > + tcf_exts_destroy(&old_r->exts); > err = tcindex_filter_result_init(old_r, cp, net); > if (err < 0) { > kfree(f); Just noticed that Hawkins has sent a patch for this. Please ignore mine. Thanks!
Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+2f9183cb6f89b0e16586@syzkaller.appspotmail.com Tested on: commit: 355479c7 Merge tag 'efi-fixes-for-v6.1-4' of git://git.. git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git console output: https://syzkaller.appspot.com/x/log.txt?x=175b5b23880000 kernel config: https://syzkaller.appspot.com/x/.config?x=979161df0e247659 dashboard link: https://syzkaller.appspot.com/bug?extid=2f9183cb6f89b0e16586 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 patch: https://syzkaller.appspot.com/x/patch.diff?x=142313f3880000 Note: testing is done by a robot and is best-effort only.
diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c index 1c9eeb98d826..00a6c04a4b42 100644 --- a/net/sched/cls_tcindex.c +++ b/net/sched/cls_tcindex.c @@ -479,6 +479,7 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base, } if (old_r && old_r != r) { + tcf_exts_destroy(&old_r->exts); err = tcindex_filter_result_init(old_r, cp, net); if (err < 0) { kfree(f);
syzkaller reported a memleak: https://syzkaller.appspot.com/bug?id=e061e6cd46417ee6566dc249d8f982c0b5977a52 unreferenced object 0xffff888107813900 (size 256): backtrace: kcalloc include/linux/slab.h:636 [inline] tcf_exts_init include/net/pkt_cls.h:250 [inline] tcindex_set_parms+0xa7/0xbe0 net/sched/cls_tcindex.c:342 tcindex_change+0xdf/0x120 net/sched/cls_tcindex.c:553 tc_new_tfilter+0x4f2/0x1100 net/sched/cls_api.c:2147 ... The reproduce calls tc_new_tfilter() continuously: tc_new_tfilter()... tcindex_set_parms() tcf_exts_init(&e, ...) // alloc e->actions tcf_exts_change(&r->exts, &e) tc_new_tfilter()... tcindex_set_parms() old_r = r // same as first r tcindex_filter_result_init(old_r, cp, net); // old_r is holding e->actions but here it calls memset(old_r, 0) // so the previous e->actions is leaked So here tcf_exts_destroy() should be called to free old_r->exts.actions before memset(old_r, 0) sets it to NULL. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Cc: stable@vger.kernel.org Reported-by: syzbot+2f9183cb6f89b0e16586@syzkaller.appspotmail.com Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com> --- #syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 355479c70a48 --- net/sched/cls_tcindex.c | 1 + 1 file changed, 1 insertion(+)