linux-kernel - Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when requested by drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 Oct 2017 17:42:14 +0200
From:   Greg KH <greg@...ah.com>
To:     Mario.Limonciello@...l.com
Cc:     dvhart@...radead.org, andy.shevchenko@...il.com,
        linux-kernel@...r.kernel.org, platform-driver-x86@...r.kernel.org,
        luto@...nel.org, quasisec@...gle.com, pali.rohar@...il.com,
        rjw@...ysocki.net, mjg59@...gle.com, hch@....de
Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices
 when requested by drivers

On Thu, Oct 05, 2017 at 02:35:43PM +0000, Mario.Limonciello@...l.com wrote:
> > 
> > > +		strcpy(buf, "wmi/");
> > > +		strcpy(buf + 4, wdriver->driver.name);
> > > +		wblock->misc_dev.minor = MISC_DYNAMIC_MINOR;
> > > +		wblock->misc_dev.name = buf;
> > > +		wblock->misc_dev.fops = &wmi_fops;
> > > +		ret = misc_register(&wblock->misc_dev);
> > > +		if (ret) {
> > > +			dev_warn(dev, "failed to register char dev: %d", ret);
> > > +			kfree(buf);
> > 
> > Again, no unwinding needed?  Error message value returned?
> 
> It comes down to if the character device should be considered optional.  I'll
> make it fail if it can't create it.

Given that you are relying on it in this patch, it would be good to fail
if something is going wrong.

> > > +	if (wdriver->file_operations) {
> > > +		kfree(wblock->misc_dev.name);
> > > +		misc_deregister(&wblock->misc_dev);
> > 
> > Unregister before freeing the device name, right?
> 
> Well if you unregister and then free the name you'll have lost the pointer.
> So isn't that the right order?

Yes, but save the pointer to the name and then free it after the larger
structure is gone.  misc_deregister() does not free anything, so you
still have the memory around here, no need to even use a temp variable.

> > > --- /dev/null
> > > +++ b/include/uapi/linux/wmi.h
> > > @@ -0,0 +1,10 @@
> > > +#ifndef _UAPI_LINUX_WMI_H
> > > +#define _UAPI_LINUX_WMI_H
> > > +
> > > +#define WMI_IOC 'W'
> > > +#define WMI_IO(instance)	_IO(WMI_IOC, instance)
> > > +#define WMI_IOR(instance)	_IOR(WMI_IOC, instance, void*)
> > > +#define WMI_IOW(instance)	_IOW(WMI_IOC, instance, void*)
> > > +#define WMI_IOWR(instance)	_IOWR(WMI_IOC, instance, void*)
> > 
> > Ugh, void *, this is going to be "fun"...
> > 
> > My comments on just how fun is left for the actual driver that attempted
> > to implement these...
> > 
> 
> So until in kernel MOF parsing is available you can't predict the format of
> what an individual ACPI method will expect for its input.  Even when the in
> kernel MOF parsing is made available the data types may be complex structures.


I have no idea what MOF is, what "parsing" is involved, or how in the
world ACPI got involved here...

good luck!

greg k-h