On Tue, Jun 1, 2021 at 9:46 PM Dan Carpenter dan.carpenter@oracle.com 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@oracle.com 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