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

[Greg KH]

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