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

[Fei Zhang]

Dear Greg,

Mostly, "class_creat" is used in kernel driver module, basically
read-only strings,
but it is easier to use a local variable string. When writing drive module,
it fails to judge the local variable string which cannot be passed in
only via interface.
I found that someone else may also face the same problem.

If we have 2 identical hardwares with different internal logic(fpga),
it may be more
appropriate to create dynamic classes according to the logical functions.

Thanks,
Fei

2020-04-06 13:41 GMT+08:00, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
> On Mon, Apr 06, 2020 at 01:33:03PM +0800, Fei Zhang wrote:
>> 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.
>
> Please do not top-post.
>
> Anyway, yes, that does seem to be a semi-valid way of creating a class,
> does anyone currently do that in the kernel tree today?  Typically
> creating classes is rare, and do not have a "dynamic name" like your
> example above, because that is not what a class is for.
>
> So while the idea is good to solve this, I would like to go back to your
> example to find out why you are doing this in the first place, as that
> does not seem to be the way to use the driver model correctly.
>
> thanks,
>
> greg k-h
>