linux-kernel - Re: [PATCH v3] net: sched: fix memory leak in tcindex_set

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y5T6Mrb7cs6o/BqS@pop-os.localdomain>
Date:   Sat, 10 Dec 2022 13:29:22 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Hawkins Jiawei <yin31149@...il.com>
Cc:     18801353760@....com, cong.wang@...edance.com, davem@...emloft.net,
        dvyukov@...gle.com, edumazet@...gle.com, jhs@...atatu.com,
        jiri@...nulli.us, kuba@...nel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, pabeni@...hat.com,
        syzbot+232ebdbd36706c965ebf@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v3] net: sched: fix memory leak in tcindex_set_parms

On Mon, Dec 05, 2022 at 11:19:56PM +0800, Hawkins Jiawei wrote:
> To be more specific, the simplified logic about original
> tcindex_set_parms() is as below:
> 
> static int
> tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> 		  u32 handle, struct tcindex_data *p,
> 		  struct tcindex_filter_result *r, struct nlattr **tb,
> 		  struct nlattr *est, u32 flags, struct netlink_ext_ack *extack)
> {
> 	...
> 	if (p->perfect) {
> 		int i;
> 
> 		if (tcindex_alloc_perfect_hash(net, cp) < 0)
> 			goto errout;
> 		cp->alloc_hash = cp->hash;
> 		for (i = 0; i < min(cp->hash, p->hash); i++)
> 			cp->perfect[i].res = p->perfect[i].res;
> 		balloc = 1;
> 	}
> 	cp->h = p->h;
> 
> 	...
> 
> 	if (cp->perfect)
> 		r = cp->perfect + handle;

We can reach here if p->perfect is non-NULL.

> 	else
> 		r = tcindex_lookup(cp, handle) ? : &new_filter_result;
> 
> 	if (old_r && old_r != r) {
> 		err = tcindex_filter_result_init(old_r, cp, net);
> 		if (err < 0) {
> 			kfree(f);
> 			goto errout_alloc;
> 		}
> 	}
> 	...
> }
> 
> - cp's h field is directly copied from p's h field
> 
> - if `old_r` is retrieved from struct tcindex_filter, in other word,
> is retrieved from p's h field. Then the `r` should get the same value
> from `tcindex_loopup(cp, handle)`.

See above, 'r' can be 'cp->perfect + handle' which is newly allocated,
hence different from 'old_r'.

> 
> - so `old_r == r` is true, code will never uses tcindex_filter_result_init()
> to clear the old_r in such case.

Not always.

> 
> So I think this patch still can fix this memory leak caused by 
> tcindex_filter_result_init(), But maybe I need to improve my
> commit message.
> 

I think your patch may introduce other memory leaks and 'old_r' may
be left as obsoleted too.

Thanks.