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

[Greg KH <gregkh@linuxfoundation.org>]

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