linux-kernel - Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led

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: <20210531044022.GU24442@kadam>
Date:   Mon, 31 May 2021 07:40:22 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Dongliang Mu <mudongliangabcd@...il.com>
Cc:     perex@...ex.cz, tiwai@...e.com, alsa-devel@...a-project.org,
        linux-kernel <linux-kernel@...r.kernel.org>,
        syzbot+08a7d8b51ea048a74ffb@...kaller.appspotmail.com
Subject: Re: [PATCH] ALSA: control led: fix memory leak in
 snd_ctl_led_register

On Mon, May 31, 2021 at 11:03:36AM +0800, Dongliang Mu wrote:
> On Sat, May 29, 2021 at 5:35 AM 慕冬亮 <mudongliangabcd@...il.com> wrote:
> >
> >
> >
> > > On May 28, 2021, at 10:05 PM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> > >
> > > On Fri, May 28, 2021 at 09:50:49PM +0800, Dongliang Mu wrote:
> > >>
> > >> Can you please give some advise on how to fix this WARN issue?
> > >
> > > But it feels like it spoils the fun if I write the commit...  Anyway:
> >
> > It’s fine. I am still in the learning process. It’s also good to learn experience by comparing your patch and my patch.
> >
> > >
> > > regards,
> > > dan carpenter
> > >
> > > diff --git a/sound/core/control_led.c b/sound/core/control_led.c
> > > index 25f57c14f294..dd357abc1b58 100644
> > > --- a/sound/core/control_led.c
> > > +++ b/sound/core/control_led.c
> > > @@ -740,6 +740,7 @@ static int __init snd_ctl_led_init(void)
> > >                       for (; group > 0; group--) {
> > >                               led = &snd_ctl_leds[group - 1];
> > >                               device_del(&led->dev);
> > > +                             device_put(&led->dev);
> > >                       }
> > >                       device_del(&snd_ctl_led_dev);
> > >                       return -ENOMEM;
> > > @@ -768,6 +769,7 @@ static void __exit snd_ctl_led_exit(void)
> > >       for (group = 0; group < MAX_LED; group++) {
> > >               led = &snd_ctl_leds[group];
> > >               device_del(&led->dev);
> > > +             device_put(&led->dev);
> > >       }
> > >       device_del(&snd_ctl_led_dev);
> > >       snd_ctl_led_clean(NULL);
> 
> Hi Dan,
> 
> I tried this patch, and it still triggers the memleak.

Did your patch fix the leak?  Because my patch should have been
equivalent except for it fixes an additional leak in the snd_ctl_led_init()
error path.

> My
> understanding is that the device object is already freed in the
> snd_ctl_led_sysfs_remove.
> 

"Already freed"?  Is it a memleak or is it a double free???  I probably
should have read the syzbot email on this...  But you didn't include
a link to it or a reported-by tag so I don't have a way to look at the
actual bug.

I did fix a bug, though...  Just not the one from the report I guess.
Please send a link to the bug report so I can look at that.  ;)

regards,
dan carpenter