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

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

[zhangfeionline]

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.

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;
 
 	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;
+	}
+
 	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);
 error:
 	kfree(cls);
+done:
 	return ERR_PTR(retval);
 }
 EXPORT_SYMBOL_GPL(__class_create);
-- 
2.7.4

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

[Greg KH]

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

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@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
>

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

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@linuxfoundation.org>:
> 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
>

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

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Mon, Apr 06, 2020 at 03:40:41PM +0800, Fei Zhang wrote:
> 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.

An individual driver should NOT be creating a class, that is not what it
is there for.

Class names are very "rare" and should not be dynamically created at
all.

> 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.

No it is not appropriate at all to do that.

So, I'll ignore this patch as this is not something that you all should
do.  If an in-kernel user needs this, I will be glad to revisit this
issue, so I strongly suggest you work to get your code merged upstream
properly, so we can review it and suggest what you should be doing
instead.

thanks,

greg k-h

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

[宋牧春]

Hi Greg,

Greg KH <gregkh@linuxfoundation.org> 于2020年4月6日周一 下午4:29写道：
>
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>
> On Mon, Apr 06, 2020 at 03:40:41PM +0800, Fei Zhang wrote:
> > 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.
>
> An individual driver should NOT be creating a class, that is not what it
> is there for.

If someone want to create a virtual device, someone can call device_create().
But the first argument is type of 'struct class *class', so we have to
call class_create()
before create device. So an individual driver may be creating a class, right?

>
> Class names are very "rare" and should not be dynamically created at
> all.

I have reviewed the code of the kstrdup_const() which is just below.

const char *kstrdup_const(const char *s, gfp_t gfp)
{
        if (is_kernel_rodata((unsigned long)s))
                return s;

        return kstrdup(s, gfp);
}

A readonly string which is in the kernel rodata, so we do not need to
dynamically allocate
memory to store the name. So with this patch applied, there is nothing
changed which
means that we did not waste memory. But it can prevent someone from
reading stale name
if an unaware user passes an address to a stack-allocated buffer.

So I think it is worth fixing, right?

-- 
Yours,
Muchun

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

[Greg KH]

On Mon, Apr 06, 2020 at 06:42:46PM +0800, 宋牧春 wrote:
> Hi Greg,
> 
> Greg KH <gregkh@linuxfoundation.org> 于2020年4月6日周一 下午4:29写道：
> >
> > A: http://en.wikipedia.org/wiki/Top_post
> > Q: Were do I find info about this thing called top-posting?
> > A: Because it messes up the order in which people normally read text.
> > Q: Why is top-posting such a bad thing?
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> >
> > A: No.
> > Q: Should I include quotations after my reply?
> >
> > http://daringfireball.net/2007/07/on_top
> >
> > On Mon, Apr 06, 2020 at 03:40:41PM +0800, Fei Zhang wrote:
> > > 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.
> >
> > An individual driver should NOT be creating a class, that is not what it
> > is there for.
> 
> If someone want to create a virtual device, someone can call device_create().
> But the first argument is type of 'struct class *class', so we have to
> call class_create()
> before create device. So an individual driver may be creating a class, right?

Again, they should not be, as classes are not what a driver creates.  It
is what a subsystem creates, as a class is a type of common devices that
all talk to userspace in the same way.

> > Class names are very "rare" and should not be dynamically created at
> > all.
> 
> I have reviewed the code of the kstrdup_const() which is just below.
> 
> const char *kstrdup_const(const char *s, gfp_t gfp)
> {
>         if (is_kernel_rodata((unsigned long)s))
>                 return s;
> 
>         return kstrdup(s, gfp);
> }
> 
> A readonly string which is in the kernel rodata, so we do not need to
> dynamically allocate
> memory to store the name. So with this patch applied, there is nothing
> changed which
> means that we did not waste memory. But it can prevent someone from
> reading stale name
> if an unaware user passes an address to a stack-allocated buffer.
> 
> So I think it is worth fixing, right?

Again, there is nothing to "fix" here as there is no code in the kernel
tree today calling this api with a class name that is not static.

If we have code that does need to do this, and it is submitted for
merging, and I agree with how it is creating the class names, I will be
glad to take a patch at that time to make this change.  Until then, this
is just added complexity for no benefit at all.

thanks,

greg k-h

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

[Greg KH]

On Tue, Apr 07, 2020 at 10:42:30PM +0800, Fei Zhang wrote:
> Dear  Greg
> 
>    Greg KH < gregkh@linuxfoundation.org >于2020年4月6日周一下午7:16写道：
> 
> > On Mon, Apr 06, 2020 at 06:42:46PM +0800,宋牧春wrote:
> > > Hi Greg,
> > >
> > > Greg KH < gregkh@linuxfoundation.org >于2020年4月6日周一下午4:29写道：
> > > >
> > > > A: http://en.wikipedia.org/wiki/ Top_post
> > <http://en.wikipedia.org/wiki/Top_post>
> > > > Q: Were do I find info about this thing called top-posting?
> > > > A: Because it messes up the order in which people normally read text.
> > > > Q: Why is top-posting such a bad thing?
> > > > A: Top-posting.
> > > > Q: What is the most annoying thing in e-mail?
> > > >
> > > > A: No.
> > > > Q : Should I include quotations after my reply?
> > > >
> > > > http://daringfireball.net/ 2007/07/on_top
> > <http://daringfireball.net/2007/07/on_top>
> > > >
> > > > On Mon, Apr 06, 2020 at 03:40:41PM +0800, Fei Zhang wrote:
> > > > > 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.
> > > >
> > > > An individual driver should NOT be creating a class, that is not what
> > it
> > > > is there for.
> > >
> > > If someone want to create a virtual device,someone can call
> > device_create().
> > > But the first argument is type of 'struct class *class', so we have to
> > > call class_create()
> > > before create device. So an individual driver may be creating a class,
> > right?
> >
> > Again, they should not be, as classes are not what a driver creates. It
> > is what a subsystem creates, as a class is a type of common devices that
> > all talk to userspace in the same way.
> >
> > > > Class names are very "rare" and should not be dynamically created at
> > > > all.
> > >
> > > I have reviewed the code of the kstrdup_const() which is just below.
> > >
> > > const char *kstrdup_const(const char *s, gfp_t gfp)
> > > {
> > > if (is_kernel_rodata((unsigned long)s))
> > >return s;
> > >
> > > return kstrdup(s, gfp);
> > > }
> > >
> > > A readonly string which is in the kernel rodata, so we do not need to
> > > dynamically allocate
> > > memory to store the name. So with this patch applied, there is nothing
> > > changed which
> > > means that we did not waste memory. But it can prevent someone from
> > > reading stale name
> > > if an unaware user passes an address to a stack-allocated buffer.
> > >
> > > So I think it is worth fixing, right?
> >
> > Again, there is nothing to "fix" here as there is no code in the kernel
> > tree today calling this api with a class name that is not static.
> >
> > If we have code that does need to do this,and it is submitted for
> > merging, and I agree with how it is creating the class names, I will be
> > glad to take a patch at that time to make this change. Until then, this
> > is just added complexity for no benefit at all.
> 
> 
> 
> 
> 
> The interface was used by many drivers. Please refer to below link.
> 
>  https://elixir.bootlin.com/linux/latest/source/drivers/char/dsp56k.c#L507

That should just be fixed up to use the misc device interface, I'll put
it on my list of things to fix...

> > https://elixir.bootlin.com/linux/latest/source/drivers/char/pcmcia/cm4040_cs.c#L654

Does anyone still care/use pcmcia drivers?  I doubt you will ever run
this code :)

> > https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/tpm-interface.c#L462

TPM is a valid class, nothing is wrong with that.

>   ...
> >
> Normally, class shall be created before creating the virtual device.
> > https://elixir.bootlin.com/linux/latest/source/fs/fuse/cuse.c#L628
> 
> https://elixir.bootlin.com/linux/latest/source/fs/pstore/pmsg.c#L66
> 

Those too are fine, nothing broken with them.

>   ...
> >
> I think it is worth fixing, it will make the code more stable.

The code is working just fine as-is, nothing is broken.  By adding
unneeded complexity, it will be more unstable.

Not to mention the first attempt didn't even get it correct, which if I
had accepted, would have _introduced_ a bug for no reason at all.

Again, if you have an in-kernel user that wants to somehow create a
class dynamically off of the stack like your example showed, I will be
glad to revisit this, after I review that driver's code.

thanks,

greg k-h

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

[宋牧春]

Hi PengfeiZhang,

Greg KH <gregkh@linuxfoundation.org> 于2020年4月6日周一 上午12:40写道：
>
> 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

Yeah, you have some problem with this fix patch which Greg mentioned.
Can you fix it? And
send the fix patch v2 to Greg?

-- 
Yours,
Muchun