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: <Y4uvQA2xxtJXltSM@pop-os.localdomain>
Date:   Sat, 3 Dec 2022 12:19:12 -0800
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Hawkins Jiawei <yin31149@...il.com>
Cc:     Jamal Hadi Salim <jhs@...atatu.com>, Jiri Pirko <jiri@...nulli.us>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>, 18801353760@....com,
        syzbot+232ebdbd36706c965ebf@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com,
        Cong Wang <cong.wang@...edance.com>,
        Dmitry Vyukov <dvyukov@...gle.com>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] net: sched: fix memory leak in tcindex_set_parms

On Tue, Nov 29, 2022 at 10:52:49AM +0800, Hawkins Jiawei wrote:
> Kernel uses tcindex_change() to change an existing
> filter properties. During the process of changing,
> kernel uses tcindex_alloc_perfect_hash() to newly
> allocate filter results, uses tcindex_filter_result_init()
> to clear the old filter result.
> 
> Yet the problem is that, kernel clears the old
> filter result, without destroying its tcf_exts structure,
> which triggers the above memory leak.
> 
> Considering that there already extis a tc_filter_wq workqueue
> to destroy the old tcindex_data by tcindex_partial_destroy_work()
> at the end of tcindex_set_parms(), this patch solves this memory
> leak bug by removing this old filter result clearing part,
> and delegating it to the tc_filter_wq workqueue.

Hmm?? The tcindex_partial_destroy_work() is to destroy 'oldp' which is
different from 'old_r'. I mean, you seem assuming that struct
tcindex_filter_result is always from struct tcindex_data, which is not
true, check the following tcindex_lookup() which retrieves tcindex_filter_result
from struct tcindex_filter.

static struct tcindex_filter_result *tcindex_lookup(struct tcindex_data *p,
                                                    u16 key)
{
        if (p->perfect) {
                struct tcindex_filter_result *f = p->perfect + key;

                return tcindex_filter_is_set(f) ? f : NULL;
        } else if (p->h) {
                struct tcindex_filter __rcu **fp;
                struct tcindex_filter *f;

                fp = &p->h[key % p->hash];
                for (f = rcu_dereference_bh_rtnl(*fp);
                     f;
                     fp = &f->next, f = rcu_dereference_bh_rtnl(*fp))
                        if (f->key == key)
                                return &f->result;
        }

        return NULL;
}

 
> diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> index 1c9eeb98d826..3f4e7a6cdd96 100644
> --- a/net/sched/cls_tcindex.c
> +++ b/net/sched/cls_tcindex.c
> @@ -478,14 +478,6 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
>  		tcf_bind_filter(tp, &cr, base);
>  	}
>  
> -	if (old_r && old_r != r) {
> -		err = tcindex_filter_result_init(old_r, cp, net);
> -		if (err < 0) {
> -			kfree(f);
> -			goto errout_alloc;
> -		}
> -	}
> -

Even if your above analysis is correct, 'old_r' becomes unused (set but not used)
now, I think you should get some compiler warning.

Thanks.