Re: [PATCH] ALSA: control led: fix memory leak in snd_ctl_led_register

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
>
> On Tue, Jun 01, 2021 at 09:17:04PM +0800, Dongliang Mu wrote:
> > On Mon, May 31, 2021 at 7:02 PM Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> > > > @@ -701,6 +706,7 @@ static void snd_ctl_led_sysfs_remove(struct snd_card *card)
> > > >               sysfs_remove_link(&card->ctl_dev.kobj, link_name);
> > > >               sysfs_remove_link(&led_card->dev.kobj, "card");
> > > >               device_del(&led_card->dev);
> > > > +             put_device(&led_card->dev);
> > > >               kfree(led_card);
> > > >               led->cards[card->number] = NULL;
> > > >       }
> > >
> > > Btw, I have created a Smatch warning for this type of code where we
> > > have:
> > >
> > >         put_device(&foo->dev);
> > >         kfree(foo);
> >
> > I don't think this should be a bug pattern. put_device will drop the
> > final reference of one object with struct device and invoke
> > device_release to release some resources.
> >
> > The release function should only clean up the internal resources in
> > the device object. It should not touch the led_card which contains the
> > device object.
> >
>
> It's only a use after free if you turn CONFIG_DEBUG_KOBJECT_RELEASE
> debugging on, which you would never do in a production environment.  The
> put_device() function calls kobject_release():

This is interesting. Let's dig a little deeper.

>
> lib/kobject.c
>    725  static void kobject_release(struct kref *kref)
>    726  {
>    727          struct kobject *kobj = container_of(kref, struct kobject, kref);
>    728  #ifdef CONFIG_DEBUG_KOBJECT_RELEASE
>    729          unsigned long delay = HZ + HZ * (get_random_int() & 0x3);
>    730          pr_info("kobject: '%s' (%p): %s, parent %p (delayed %ld)\n",
>    731                   kobject_name(kobj), kobj, __func__, kobj->parent, delay);
>    732          INIT_DELAYED_WORK(&kobj->release, kobject_delayed_cleanup);
>                                                   ^^^^^^^^^^^^^^^^^^^^^^^
>
>    733
>    734          schedule_delayed_work(&kobj->release, delay);
>    735  #else
>    736          kobject_cleanup(kobj);
>    737  #endif
>    738  }
>
> This release will be done later and it references led_card->dev which is
> now freed.

The call chain of kobject_delayed_cleanup is kobject_delayed_cleanup
-> kobject_cleanup. From the comment, kobject_cleanup should only
clean the resources in the kobject, without touching the dev object.
To further confirm, I checked the implementation and found out there
seem no references to the dev object. Would you mind pointing out the
reference to dev object? Moreover, if kobject_cleanup touches the
resources out of kobject, shall we directly change this function other
than its callees?

#ifdef CONFIG_DEBUG_KOBJECT_RELEASE
static void kobject_delayed_cleanup(struct work_struct *work)
{
kobject_cleanup(container_of(to_delayed_work(work),
    struct kobject, release));
}
#endif

/*
 * kobject_cleanup - free kobject resources.
 * @kobj: object to cleanup
 */
static void kobject_cleanup(struct kobject *kobj)
{
struct kobject *parent = kobj->parent;
struct kobj_type *t = get_ktype(kobj);
const char *name = kobj->name;

pr_debug("kobject: '%s' (%p): %s, parent %p\n",
kobject_name(kobj), kobj, __func__, kobj->parent);

if (t && !t->release)
pr_debug("kobject: '%s' (%p): does not have a release() function, it
is broken and must be fixed. See
Documentation/core-api/kobject.rst.\n",
kobject_name(kobj), kobj);

/* remove from sysfs if the caller did not do it */
if (kobj->state_in_sysfs) {
pr_debug("kobject: '%s' (%p): auto cleanup kobject_del\n",
kobject_name(kobj), kobj);
__kobject_del(kobj);
} else {
/* avoid dropping the parent reference unnecessarily */
parent = NULL;
}

if (t && t->release) {
pr_debug("kobject: '%s' (%p): calling ktype release\n",
kobject_name(kobj), kobj);
t->release(kobj);
}

/* free name if we allocated it */
if (name) {
pr_debug("kobject: '%s': free name\n", name);
kfree_const(name);
}

kobject_put(parent);
}

>
> The Smatch check did work pretty decently.  These are all use after free
> bugs if you have CONFIG_DEBUG_KOBJECT_RELEASE enabled.  (Line numbers
> from Friday's linux-next).  I'm not going to bother fixing them because
> they're only an issue for CONFIG_DEBUG_KOBJECT_RELEASE and not for
> production but I will email people when more of these bugs are
> introduced.
>
> sound/core/control_led.c:688 snd_ctl_led_sysfs_add() warn: freeing device managed memory (UAF): 'led_card'
> drivers/usb/gadget/function/f_mass_storage.c:2649 fsg_common_remove_lun() warn: freeing device managed memory (UAF): 'lun'
> drivers/usb/gadget/function/f_mass_storage.c:2818 fsg_common_create_lun() warn: freeing device managed memory (UAF): 'lun'
> drivers/usb/gadget/function/f_mass_storage.c:2881 fsg_common_release() warn: freeing device managed memory (UAF): 'lun'
> drivers/w1/w1.c:810 w1_unref_slave() warn: freeing device managed memory (UAF): 'sl'
> drivers/pci/endpoint/pci-epc-core.c:671 pci_epc_destroy() warn: freeing device managed memory (UAF): 'epc'
> drivers/pci/endpoint/pci-epc-core.c:742 __pci_epc_create() warn: freeing device managed memory (UAF): 'epc'
> drivers/infiniband/ulp/srp/ib_srp.c:3930 srp_add_port() warn: freeing device managed memory (UAF): 'host'
> drivers/infiniband/ulp/srp/ib_srp.c:4058 srp_remove_one() warn: freeing device managed memory (UAF): 'host'
> drivers/infiniband/ulp/rtrs/rtrs-clt.c:2695 alloc_clt() warn: freeing device managed memory (UAF): 'clt'
> drivers/media/pci/solo6x10/solo6x10-core.c:156 free_solo_dev() warn: freeing device managed memory (UAF): 'solo_dev'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:203 nfp_cpp_free() warn: freeing device managed memory (UAF): 'cpp'
> drivers/net/ethernet/netronome/nfp/nfpcore/nfp_cppcore.c:1258 nfp_cpp_from_operations() warn: freeing device managed memory (UAF): 'cpp'
> drivers/net/netdevsim/bus.c:354 nsim_bus_dev_del() warn: freeing device managed memory (UAF): 'nsim_bus_dev'
> drivers/thermal/thermal_core.c:1002 __thermal_cooling_device_register() warn: freeing device managed memory (UAF): 'cdev'
>
> regards,
> dan carpenter
>



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux