netdev - Re: [PATCH] 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]

Date:   Fri,  4 Nov 2022 00:07:00 +0800
From:   Hawkins Jiawei <yin31149@...il.com>
To:     kuba@...nel.org
Cc:     18801353760@....com, davem@...emloft.net, edumazet@...gle.com,
        jhs@...atatu.com, jiri@...nulli.us, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, pabeni@...hat.com,
        syzbot+232ebdbd36706c965ebf@...kaller.appspotmail.com,
        syzkaller-bugs@...glegroups.com, xiyou.wangcong@...il.com,
        yin31149@...il.com
Subject: Re: [PATCH] net: sched: fix memory leak in tcindex_set_parms

Hi Jakub,
On Thu, 3 Nov 2022 at 11:26, Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Mon, 31 Oct 2022 14:08:35 +0800 Hawkins Jiawei wrote:
> > Kernel will uses tcindex_change() to change an existing
>
> s/will//
>
> > traffic-control-indices filter properties. During the
> > process of changing, kernel will clears the old
>
> s/will//
>
> > traffic-control-indices filter result, and updates it
> > by RCU assigning new traffic-control-indices data.
> >
> > Yet the problem is that, kernel will clears the old
>
> s/will//
Thanks for the suggestion. I will amend these in the v2 patch.

>
> > traffic-control-indices filter result, without destroying
> > its tcf_exts structure, which triggers the above
> > memory leak.
> >
> > This patch solves it by using tcf_exts_destroy() to
> > destroy the tcf_exts structure in old
> > traffic-control-indices filter result.
> >
>
> Please provide a Fixes tag to where the problem was introduced
> (or the initial git commit).
Thanks for reminding, it seems that the problem was 
introduced by commit 
b9a24bb76bf6 ("net_sched: properly handle failure case of tcf_exts_init()"),
because it was in this commit that kernel allocated the struct tcf_exts
for new traffic-control-indices filter result in tcindex_alloc_perfect_hash().

I will add the tag in the v2 patch.

>
> > Link: https://lore.kernel.org/all/0000000000001de5c505ebc9ec59@google.com/
> > Reported-by: syzbot+232ebdbd36706c965ebf@...kaller.appspotmail.com
> > Tested-by: syzbot+232ebdbd36706c965ebf@...kaller.appspotmail.com
> > Signed-off-by: Hawkins Jiawei <yin31149@...il.com>
> > ---
> >  net/sched/cls_tcindex.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
> > index 1c9eeb98d826..dc872a794337 100644
> > --- a/net/sched/cls_tcindex.c
> > +++ b/net/sched/cls_tcindex.c
> > @@ -338,6 +338,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> >       struct tcf_result cr = {};
> >       int err, balloc = 0;
> >       struct tcf_exts e;
> > +#ifdef CONFIG_NET_CLS_ACT
> > +     struct tcf_exts old_e = {};
> > +#endif
>
> Why all the ifdefs?
Thanks for suggestion, it seems that these ifdefs are not needed.
I will delete these in the v2 patch.

>
> >       err = tcf_exts_init(&e, net, TCA_TCINDEX_ACT, TCA_TCINDEX_POLICE);
> >       if (err < 0)
> > @@ -479,6 +482,14 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> >       }
> >
> >       if (old_r && old_r != r) {
> > +#ifdef CONFIG_NET_CLS_ACT
> > +             /* r->exts is not copied from old_r->exts, and
> > +              * the following code will clears the old_r, so
> > +              * we need to destroy it after updating the tp->root,
> > +              * to avoid memory leak bug.
> > +              */
> > +             old_e = old_r->exts;
> > +#endif
>
> Can't you localize all the changes to this if block?
>
> Maybe add a function called tcindex_filter_result_reinit()
> which will act more appropriately?
I think we shouldn't put the tcf_exts_destroy(&old_e)
into this if block, or other RCU readers may derefer the
freed memory (Please correct me If I am wrong).

So I put the tcf_exts_destroy(&old_e) near the tcindex 
destroy work, after the RCU updateing.

>
> >               err = tcindex_filter_result_init(old_r, cp, net);
> >               if (err < 0) {
> >                       kfree(f);
> > @@ -510,6 +521,9 @@ tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
> >               tcf_exts_destroy(&new_filter_result.exts);
> >       }
> >
> > +#ifdef CONFIG_NET_CLS_ACT
> > +     tcf_exts_destroy(&old_e);
> > +#endif
> >       if (oldp)
> >               tcf_queue_work(&oldp->rwork, tcindex_partial_destroy_work);
> >       return 0;