Re: [PATCH] driver core: Fix possible use after free on name

Fei Zhang
2020-04-06T05:33:10Z

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
>