Messages in this thread | | | Date | Tue, 7 Apr 2020 17:01:23 +0200 | From | Greg KH <> | Subject | Re: [External] Re: [PATCH] driver core: Fix possible use after free on name |
| |
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
| |