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@xxxxxxxxxxxxxxxxxxx >ä2020å4æ6æåääå7:16åéï
> 
> > On Mon, Apr 06, 2020 at 06:42:46PM +0800,åçæwrote:
> > > Hi Greg,
> > >
> > > Greg KH < gregkh@xxxxxxxxxxxxxxxxxxx >ä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