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

[Fei Zhang]

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@xxxxxxxxxxxxxxxxxxx>:
> On Sun, Apr 05, 2020 at 09:05:49AM -0700, zhangfeionline@xxxxxxxxx wrote:
>> From: PengfeiZhang <zhangfeionline@xxxxxxxxx>
>>
>> __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@xxxxxxxxx>
>> ---
>>  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
>