Messages in this thread Patch in this message | | | From | Fei Zhang <> | Date | Mon, 6 Apr 2020 13:33:03 +0800 | Subject | Re: [PATCH] driver core: Fix possible use after free on name |
| |
Dear Greg,
Please refer to below for the crash. If you are fine with it, I will submit another patch with correcting the error mentioned.
When writing a kernel driver module, we may use it like this:
static int frv_init(int index) { ... char name [128]={0}; sprintf(name,"test_%d",index); mount_dev_p->pci_class =class_create(THIS_MODULE,name); classdev = device_create(mount_dev_p->pci_class, NULL, devno, NULL, "PCIE_%x",index); ... }
static void frv_exit(void) { ... device_destroy(mount_dev_p->pci_class,mount_dev_p->devno); class_destroy(mount_dev_p->pci_class ); ... }
But when we remove the module, a crash occurres when calling device_destroy.
Call Trace: vsnprintf+0x2b2/0x4e0 add_uevent_var+0x7d/0x110 kobject_uevent_env+0x23f/0x770 kobject_uevent+0xb/0x10 device_del+0x23b/0x360 device_unregister+0x1a/0x60 device_destroy+0x3c/0x50
I traced the code and found that an invalid local variable was called in "kobject_uevent_env()", and triggered further crash as followed:
kobject_uevent_env(...) { ... struct kset_uevent_ops *uevent_ops; uevent_ops = kset->uevent_ops; subsystem = uevent_ops->name(kset, kobj); add_uevent_var(env, "SUBSYSTEM=%s", subsystem); ... }
What is the "subsystem" value, continue to track.
static const struct kset_uevent_ops device_uevent_ops = { .name = dev_uevent_name, };
static const char *dev_uevent_name(struct kset *kset, struct kobject *kobj) { struct device *dev = kobj_to_dev(kobj); if (dev->class) return dev->class->name; return NULL; }
Everything becomes clear: "class->name" and "subsystem" value is local variable array address of start module function inside "char name [128]". Used released local variable in device_destroy's kobject_uevent_env, an error occurred.
Thanks, Fei
2020-04-06 0:40 GMT+08:00, Greg KH <gregkh@linuxfoundation.org>: > On Sun, Apr 05, 2020 at 09:05:49AM -0700, zhangfeionline@gmail.com wrote: >> From: PengfeiZhang <zhangfeionline@gmail.com> >> >> __class_create() copies the pointer to the name passed as an >> argument only to be used later. But there's a chance the caller >> could immediately free the passed string(e.g., local variable). >> This could trigger a use after free when we use class name(e.g., >> dev_uevent_name()called by device_destroy(),class_create_release()). >> >> To be on the safe side: duplicate the string with kstrdup_const() >> so that if an unaware user passes an address to a stack-allocated >> buffer, we won't get the arbitrary name and crash. > > Where are you seeing this happen? > >> >> Signed-off-by: PengfeiZhang <zhangfeionline@gmail.com> >> --- >> drivers/base/class.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/base/class.c b/drivers/base/class.c >> index bcd410e..770b3b3 100644 >> --- a/drivers/base/class.c >> +++ b/drivers/base/class.c >> @@ -206,6 +206,7 @@ void class_unregister(struct class *cls) >> static void class_create_release(struct class *cls) >> { >> pr_debug("%s called for %s\n", __func__, cls->name); >> + kfree_const(cls->name); >> kfree(cls); >> } >> >> @@ -227,7 +228,10 @@ struct class *__class_create(struct module *owner, >> const char *name, >> struct lock_class_key *key) >> { >> struct class *cls; >> - int retval; >> + int retval = -EINVAL; >> + >> + if (!name) >> + goto done; > > This is a new change, who calls this function with name not being set? > > >> >> cls = kzalloc(sizeof(*cls), GFP_KERNEL); >> if (!cls) { >> @@ -235,18 +239,27 @@ struct class *__class_create(struct module *owner, >> const char *name, >> goto error; >> } >> >> + name = kstrdup_const(name, GFP_KERNEL); >> + if (!name) { >> + retval = -ENOMEM; >> + goto error; >> + } > > and overwriting the pointer like that is bad-form, try doing something > else here instead. > >> + >> cls->name = name; >> cls->owner = owner; >> cls->class_release = class_create_release; >> >> retval = __class_register(cls, key); >> if (retval) >> - goto error; >> + goto error_class_register; >> >> return cls; >> >> +error_class_register: >> + kfree(cls->name); > > kfree_const()? > > thanks, > > greg k-h >
| |