linux-kernel - Re: [PATCH] gpiolib: free device name on error path to fix kmemleak

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

Date:   Mon, 1 Feb 2021 19:36:44 +0200
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Bartosz Golaszewski <bgolaszewski@...libre.com>
Cc:     "quanyang.wang" <quanyang.wang@...driver.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] gpiolib: free device name on error path to fix kmemleak

On Mon, Feb 1, 2021 at 5:50 PM Bartosz Golaszewski
<bgolaszewski@...libre.com> wrote:
>
> On Sat, Jan 30, 2021 at 4:45 AM quanyang.wang
> <quanyang.wang@...driver.com> wrote:
> >
> > Hi Andy,
> >
> > On 1/30/21 1:26 AM, Andy Shevchenko wrote:
> > > On Fri, Jan 29, 2021 at 2:01 PM <quanyang.wang@...driver.com> wrote:
> > >> From: Quanyang Wang <quanyang.wang@...driver.com>
> > >>
> > >> In gpiochip_add_data_with_key, we should check the return value of
> > >> dev_set_name to ensure that device name is allocated successfully
> > >> and then add a label on the error path to free device name to fix
> > >> kmemleak as below:
> > > Thanks for the report.
> > > Unfortunately...
> > >
> > >> +       ret = dev_set_name(&gdev->dev, GPIOCHIP_NAME "%d", gdev->id);
> > >> +       if (ret)
> > >> +               goto err_free_ida;
> > > ...
> > >
> > >> +err_free_dev_name:
> > >> +       kfree(dev_name(&gdev->dev));
> > > ...this approach seems to  create a possible double free if I'm not mistaken.
> > Thanks for your comment.  I didn't catch the double free. Would you
> > please point it out?
> > >
> > > The idea is that device name should be cleaned in kobject ->release()
> > > callback when device is put.
> >
> > Yes, the device name should be freed by calling put_device(&gdev->dev).
> > But int gpiochip_add_data_with_key,
> >
> > when running dev_set_name, "gdev->dev.release" hasn't been installed
> > until in the tail of gpiochip_add_data_with_key.
> >
> > So we couldn't call put_device here.
> >
> > Any suggestion is much appreciated.
> >
> > Thanks,
> >
> > Quanyang
> >
> > > Can you elaborate?
> > >
>
> Andy,
>
> gdev->dev.release is assigned as the very last step in
> gpiochip_add_data_with_key() so the patch looks correct to me. Do you
> still have objections? Maybe I'm not seeing something.

OK! (Sorry, don't have time to look deeper, just remember that netdev
code used to (or still?) have some twisted cases with device
registration and similar syzcaller issue, but in that case it wasn't
so easy to fix.

-- 
With Best Regards,
Andy Shevchenko