[v4,12/14] platform/x86: wmi: create character devices when requested by drivers

Commit Message

Limonciello, Mario Oct. 4, 2017, 10:48 p.m. UTC

For WMI operations that are only Set or Query read or write sysfs
attributes created by WMI vendor drivers make sense.

For other WMI operations that are run on Method, there needs to be a
way to guarantee to userspace that the results from the method call
belong to the data request to the method call.  Sysfs attributes don't
work well in this scenario because two userspace processes may be
competing at reading/writing an attribute and step on each other's
data.

When a WMI vendor driver declares an ioctl in a file_operations object
the WMI bus driver will create a character device that maps to those
file operations.

That character device will correspond to this path:
/dev/wmi/$driver

The WMI bus driver will interpret the IOCTL calls, test them for
a valid instance and pass them on to the vendor driver to run.

This creates an implicit policy that only driver per character
device.  If a module matches multiple GUID's, the wmi_devices
will need to be all handled by the same wmi_driver if the same
character device is used.

The WMI vendor drivers will be responsible for managing access to
this character device and proper locking on it.

When a WMI vendor driver is unloaded the WMI bus driver will clean
up the character device.

Signed-off-by: Mario Limonciello <[e-mail]>
---
 MAINTAINERS                |  1 +
 drivers/platform/x86/wmi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/wmi.h        |  2 ++
 include/uapi/linux/wmi.h   | 10 +++++++
 4 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 include/uapi/linux/wmi.h

Comments

Darren Hart Oct. 5, 2017, 2:33 a.m. UTC | #1

On Wed, Oct 04, 2017 at 05:48:38PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
> 
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call.  Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
> 
> When a WMI vendor driver declares an ioctl in a file_operations object
> the WMI bus driver will create a character device that maps to those
> file operations.
> 
> That character device will correspond to this path:
> /dev/wmi/$driver
> 
> The WMI bus driver will interpret the IOCTL calls, test them for
> a valid instance and pass them on to the vendor driver to run.
> 
> This creates an implicit policy that only driver per character
> device.  If a module matches multiple GUID's, the wmi_devices
> will need to be all handled by the same wmi_driver if the same
> character device is used.
> 
> The WMI vendor drivers will be responsible for managing access to
> this character device and proper locking on it.
> 
> When a WMI vendor driver is unloaded the WMI bus driver will clean
> up the character device.
> 
> Signed-off-by: Mario Limonciello <[e-mail]>
> ---
>  MAINTAINERS                |  1 +
>  drivers/platform/x86/wmi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/wmi.h        |  2 ++
>  include/uapi/linux/wmi.h   | 10 +++++++
>  4 files changed, 79 insertions(+), 1 deletion(-)
>  create mode 100644 include/uapi/linux/wmi.h
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
...
> +static long wmi_ioctl(struct file *filp, unsigned int cmd,
> +		      unsigned long arg)
> +{
> +	struct wmi_driver *wdriver;
> +	struct wmi_block *wblock;
> +	const char *driver_name;
> +	struct list_head *p;
> +	bool found = false;
> +
> +	if (_IOC_TYPE(cmd) != WMI_IOC)
> +		return -ENOTTY;
> +
> +	driver_name = filp->f_path.dentry->d_iname;
> +
> +	list_for_each(p, &wmi_block_list) {
> +		wblock = list_entry(p, struct wmi_block, list);
> +		wdriver = container_of(wblock->dev.dev.driver,
> +			struct wmi_driver, driver);
> +		if (strcmp(driver_name, wdriver->driver.name) == 0) {
> +			found = true;
> +			break;

A bit of a nitpic, but the "found" variable isn't necessary. The wdriver
pointer is sufficient:

		if (strcmp(driver_name, wdriver->driver.name) == 0)
			break;
		wdriver = NULL;

> +	if (!found ||

	if (wdriver || ...

And you save a local variable and a couple lines.

...

>  static int wmi_dev_probe(struct device *dev)
>  {
>  	struct wmi_block *wblock = dev_to_wblock(dev);
>  	struct wmi_driver *wdriver =
>  		container_of(dev->driver, struct wmi_driver, driver);
>  	int ret = 0;
> +	char *buf;
>  
>  	if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
>  		dev_warn(dev, "failed to enable device -- probing anyway\n");
>  
> +	/* driver wants a character device made */
> +	if (wdriver->file_operations) {
> +		buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;
> +		strcpy(buf, "wmi/");
> +		strcpy(buf + 4, wdriver->driver.name);

sprintf(buf, "wmi/%s", wdriver->driver.name)

Greg KH Oct. 5, 2017, 7:16 a.m. UTC | #2

On Wed, Oct 04, 2017 at 05:48:38PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
> 
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call.  Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.

And you protect this from happening in the ioctl?  I didn't see it, but
ok, I'll take your word for it :)

> When a WMI vendor driver declares an ioctl in a file_operations object
> the WMI bus driver will create a character device that maps to those
> file operations.
> 
> That character device will correspond to this path:
> /dev/wmi/$driver
> 
> The WMI bus driver will interpret the IOCTL calls, test them for
> a valid instance and pass them on to the vendor driver to run.
> 
> This creates an implicit policy that only driver per character
> device.  If a module matches multiple GUID's, the wmi_devices
> will need to be all handled by the same wmi_driver if the same
> character device is used.

Interesting "way out" but ok, I can buy it...

> The WMI vendor drivers will be responsible for managing access to
> this character device and proper locking on it.
> 
> When a WMI vendor driver is unloaded the WMI bus driver will clean
> up the character device.
> 
> Signed-off-by: Mario Limonciello <[e-mail]>
> ---
>  MAINTAINERS                |  1 +
>  drivers/platform/x86/wmi.c | 67 +++++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/wmi.h        |  2 ++
>  include/uapi/linux/wmi.h   | 10 +++++++
>  4 files changed, 79 insertions(+), 1 deletion(-)
>  create mode 100644 include/uapi/linux/wmi.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0357e9b1cfaf..6db1d84999bc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -372,6 +372,7 @@ ACPI WMI DRIVER
>  L:	[e-mail]
>  S:	Orphan
>  F:	drivers/platform/x86/wmi.c
> +F:	include/uapi/linux/wmi.h
>  
>  AD1889 ALSA SOUND DRIVER
>  M:	Thibaut Varene <[e-mail]>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index bcb41c1c7f52..5aef052b4aab 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -38,6 +38,7 @@
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> +#include <linux/miscdevice.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
> @@ -69,6 +70,7 @@ struct wmi_block {
>  	struct wmi_device dev;
>  	struct list_head list;
>  	struct guid_block gblock;
> +	struct miscdevice misc_dev;
>  	struct acpi_device *acpi_device;
>  	wmi_notify_handler handler;
>  	void *handler_data;
> @@ -765,22 +767,80 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
>  	return 0;
>  }
>  
> +static long wmi_ioctl(struct file *filp, unsigned int cmd,
> +		      unsigned long arg)
> +{
> +	struct wmi_driver *wdriver;
> +	struct wmi_block *wblock;
> +	const char *driver_name;
> +	struct list_head *p;
> +	bool found = false;
> +
> +	if (_IOC_TYPE(cmd) != WMI_IOC)
> +		return -ENOTTY;
> +
> +	driver_name = filp->f_path.dentry->d_iname;
> +
> +	list_for_each(p, &wmi_block_list) {
> +		wblock = list_entry(p, struct wmi_block, list);
> +		wdriver = container_of(wblock->dev.dev.driver,
> +			struct wmi_driver, driver);
> +		if (strcmp(driver_name, wdriver->driver.name) == 0) {
> +			found = true;
> +			break;
> +		}
> +	}

You can provide an open() call to handle this type of logic for you, so
you don't have to do it on every ioctl() call, but I guess it's not
really a big deal, right?

> +	if (!found ||
> +	    !wdriver->file_operations ||
> +	    !wdriver->file_operations->unlocked_ioctl)
> +		return -ENODEV;

Shouldn't you check for unlocked_ioctl() already?  No need to check it
here, right?

And if you are only passing down unlocked_ioctl, there's no need for a
whole empty file_operations structure in the driver, right?  Just have
an ioctl callback to make things smaller and simpler to understand.

> +	/* make sure we're not calling a higher instance */
> +	if (_IOC_NR(cmd) > wblock->gblock.instance_count)
> +		return -EINVAL;

What exactly does this protect from?

> +	/* driver wants a character device made */
> +	if (wdriver->file_operations) {

Check for unlocked_ioctl here, actually, drop the file_operations
entirely, and just have that one callback.

> +		buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
> +		if (!buf)
> +			return -ENOMEM;

No unwinding of other logic needed?

> +		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?

> +		}
> +	}
> +
>  	if (wdriver->probe) {
>  		ret = wdriver->probe(dev_to_wdev(dev));
>  		if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
>  			dev_warn(dev, "failed to disable device\n");
>  	}
> -
>  	return ret;
>  }
>  
> @@ -791,6 +851,11 @@ static int wmi_dev_remove(struct device *dev)
>  		container_of(dev->driver, struct wmi_driver, driver);
>  	int ret = 0;
>  
> +	if (wdriver->file_operations) {
> +		kfree(wblock->misc_dev.name);
> +		misc_deregister(&wblock->misc_dev);

Unregister before freeing the device name, right?

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

greg k-h

Limonciello, Mario Oct. 5, 2017, 2:35 p.m. UTC | #3

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 2:16 AM
> To: Limonciello, Mario <[e-mail]>
> Cc: [e-mail]; Andy Shevchenko <[e-mail]>;
> LKML <[e-mail]>; [e-mail];
> Andy Lutomirski <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Wed, Oct 04, 2017 at 05:48:38PM -0500, Mario Limonciello wrote:
> > For WMI operations that are only Set or Query read or write sysfs
> > attributes created by WMI vendor drivers make sense.
> >
> > For other WMI operations that are run on Method, there needs to be a
> > way to guarantee to userspace that the results from the method call
> > belong to the data request to the method call.  Sysfs attributes don't
> > work well in this scenario because two userspace processes may be
> > competing at reading/writing an attribute and step on each other's
> > data.
> 
> And you protect this from happening in the ioctl?  I didn't see it, but
> ok, I'll take your word for it :)

The ioctl does take a mutex.

> 
> > When a WMI vendor driver declares an ioctl in a file_operations object
> > the WMI bus driver will create a character device that maps to those
> > file operations.
> >
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> >
> > The WMI bus driver will interpret the IOCTL calls, test them for
> > a valid instance and pass them on to the vendor driver to run.
> >
> > This creates an implicit policy that only driver per character
> > device.  If a module matches multiple GUID's, the wmi_devices
> > will need to be all handled by the same wmi_driver if the same
> > character device is used.
> 
> Interesting "way out" but ok, I can buy it...
> 
> > The WMI vendor drivers will be responsible for managing access to
> > this character device and proper locking on it.
> >
> > When a WMI vendor driver is unloaded the WMI bus driver will clean
> > up the character device.
> >
> > Signed-off-by: Mario Limonciello <[e-mail]>
> > ---
> >  MAINTAINERS                |  1 +
> >  drivers/platform/x86/wmi.c | 67
> +++++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/wmi.h        |  2 ++
> >  include/uapi/linux/wmi.h   | 10 +++++++
> >  4 files changed, 79 insertions(+), 1 deletion(-)
> >  create mode 100644 include/uapi/linux/wmi.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0357e9b1cfaf..6db1d84999bc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -372,6 +372,7 @@ ACPI WMI DRIVER
> >  L:	[e-mail]
> >  S:	Orphan
> >  F:	drivers/platform/x86/wmi.c
> > +F:	include/uapi/linux/wmi.h
> >
> >  AD1889 ALSA SOUND DRIVER
> >  M:	Thibaut Varene <[e-mail]>
> > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > index bcb41c1c7f52..5aef052b4aab 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -38,6 +38,7 @@
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > +#include <linux/miscdevice.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> > @@ -69,6 +70,7 @@ struct wmi_block {
> >  	struct wmi_device dev;
> >  	struct list_head list;
> >  	struct guid_block gblock;
> > +	struct miscdevice misc_dev;
> >  	struct acpi_device *acpi_device;
> >  	wmi_notify_handler handler;
> >  	void *handler_data;
> > @@ -765,22 +767,80 @@ static int wmi_dev_match(struct device *dev, struct
> device_driver *driver)
> >  	return 0;
> >  }
> >
> > +static long wmi_ioctl(struct file *filp, unsigned int cmd,
> > +		      unsigned long arg)
> > +{
> > +	struct wmi_driver *wdriver;
> > +	struct wmi_block *wblock;
> > +	const char *driver_name;
> > +	struct list_head *p;
> > +	bool found = false;
> > +
> > +	if (_IOC_TYPE(cmd) != WMI_IOC)
> > +		return -ENOTTY;
> > +
> > +	driver_name = filp->f_path.dentry->d_iname;
> > +
> > +	list_for_each(p, &wmi_block_list) {
> > +		wblock = list_entry(p, struct wmi_block, list);
> > +		wdriver = container_of(wblock->dev.dev.driver,
> > +			struct wmi_driver, driver);
> > +		if (strcmp(driver_name, wdriver->driver.name) == 0) {
> > +			found = true;
> > +			break;
> > +		}
> > +	}
> 
> You can provide an open() call to handle this type of logic for you, so
> you don't have to do it on every ioctl() call, but I guess it's not
> really a big deal, right?
> 
> > +	if (!found ||
> > +	    !wdriver->file_operations ||
> > +	    !wdriver->file_operations->unlocked_ioctl)
> > +		return -ENODEV;
> 
> Shouldn't you check for unlocked_ioctl() already?  No need to check it
> here, right?

You're right.  I'll move the check earlier in the initialization.

> 
> And if you are only passing down unlocked_ioctl, there's no need for a
> whole empty file_operations structure in the driver, right?  Just have
> an ioctl callback to make things smaller and simpler to understand.
> 

> > +	/* make sure we're not calling a higher instance */
> > +	if (_IOC_NR(cmd) > wblock->gblock.instance_count)
> > +		return -EINVAL;
> 
> What exactly does this protect from?

I'll clarify the comment, but essentially if userspace tries to make
an ioctl call on an instance that doesn't exist it should fail.

> 
> > +	/* driver wants a character device made */
> > +	if (wdriver->file_operations) {
> 
> Check for unlocked_ioctl here, actually, drop the file_operations
> entirely, and just have that one callback.

OK.

> 
> > +		buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
> > +		if (!buf)
> > +			return -ENOMEM;
> 
> No unwinding of other logic needed?

There shouldn't be.  The stuff earlier is optional.

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

> 
> > +		}
> > +	}
> > +
> >  	if (wdriver->probe) {
> >  		ret = wdriver->probe(dev_to_wdev(dev));
> >  		if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
> >  			dev_warn(dev, "failed to disable device\n");
> >  	}
> > -
> >  	return ret;
> >  }
> >
> > @@ -791,6 +851,11 @@ static int wmi_dev_remove(struct device *dev)
> >  		container_of(dev->driver, struct wmi_driver, driver);
> >  	int ret = 0;
> >
> > +	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?

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

Greg KH Oct. 5, 2017, 3:42 p.m. UTC | #4

On Thu, Oct 05, 2017 at 02:35:43PM +0000, [e-mail] 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

Pali Rohár Oct. 5, 2017, 3:51 p.m. UTC | #5

On Thursday 05 October 2017 17:42:14 Greg KH wrote:
> > > > --- /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

Hi Greg! Simple description: In ACPI is stored binary MOF buffer which
describe format (function name, parameters, sizeof and type of
parameters, etc) for all those calls.

Basically it is what should be used for checking if userspace pass
correct "buffer" via ioctl to WMI.

But that binary MOF is undocumented, invented by Microsoft... and
present in every one ACPI BIOS notebook (which uses WMI). It is
de-facto industrial standard, just tools for encoding/decoding it are
only for Microsoft Windows systems.

I was able to decipher that format and wrote simple userspace parser:
https://github.com/pali/bmfdec

Funny part is that format is not encrypted, but compressed by DMSDOS
compatible compression algorithm :-) You probably would remember old
FAT16 days with compression...

Hope that it helps you to understand it.

Greg KH Oct. 5, 2017, 4:26 p.m. UTC | #6

On Thu, Oct 05, 2017 at 05:51:56PM +0200, Pali Rohár wrote:
> On Thursday 05 October 2017 17:42:14 Greg KH wrote:
> > > > > --- /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
> 
> Hi Greg! Simple description: In ACPI is stored binary MOF buffer which
> describe format (function name, parameters, sizeof and type of
> parameters, etc) for all those calls.
> 
> Basically it is what should be used for checking if userspace pass
> correct "buffer" via ioctl to WMI.
> 
> But that binary MOF is undocumented, invented by Microsoft... and
> present in every one ACPI BIOS notebook (which uses WMI). It is
> de-facto industrial standard, just tools for encoding/decoding it are
> only for Microsoft Windows systems.
> 
> I was able to decipher that format and wrote simple userspace parser:
> https://github.com/pali/bmfdec
> 
> Funny part is that format is not encrypted, but compressed by DMSDOS
> compatible compression algorithm :-) You probably would remember old
> FAT16 days with compression...
> 
> Hope that it helps you to understand it.

It does, thanks.  And as we now understand it (I'm guessing it had to be
semi-understood in the older wmi drivers already), validating it
properly seems to be the key for creating an interface that we "know" to
be safe.

thanks,

greg k-h

Darren Hart Oct. 5, 2017, 5:39 p.m. UTC | #7

On Thu, Oct 05, 2017 at 06:26:28PM +0200, Greg KH wrote:
> On Thu, Oct 05, 2017 at 05:51:56PM +0200, Pali Rohár wrote:
> > On Thursday 05 October 2017 17:42:14 Greg KH wrote:
> > > > > > --- /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
> > 
> > Hi Greg! Simple description: In ACPI is stored binary MOF buffer which
> > describe format (function name, parameters, sizeof and type of
> > parameters, etc) for all those calls.
> > 
> > Basically it is what should be used for checking if userspace pass
> > correct "buffer" via ioctl to WMI.
> > 
> > But that binary MOF is undocumented, invented by Microsoft... and
> > present in every one ACPI BIOS notebook (which uses WMI). It is
> > de-facto industrial standard, just tools for encoding/decoding it are
> > only for Microsoft Windows systems.
> > 
> > I was able to decipher that format and wrote simple userspace parser:
> > https://github.com/pali/bmfdec
> > 
> > Funny part is that format is not encrypted, but compressed by DMSDOS
> > compatible compression algorithm :-) You probably would remember old
> > FAT16 days with compression...
> > 
> > Hope that it helps you to understand it.
> 
> It does, thanks.  And as we now understand it (I'm guessing it had to be
> semi-understood in the older wmi drivers already), validating it
> properly seems to be the key for creating an interface that we "know" to
> be safe.
> 

We don't use the MOF data in any of the existing wmi drivers, because
they are all oddities which map to kernel managed subsystems (hotkeys,
LED control, RF Kill switches) rather than what WMI (Windows
Manageability Interface) was designed for. The intent of these patches
to enable that management aspect of the platform.

This is the biggest hurdle for WMI support.

WMI was designed to bypass the OS, and is used in consumer devices
intended to run Windows. This leads to an interface that is very vendor
specific and not consistently broken up into nice functional blocks.

Vendors would like to use this interface in Linux as it is being used in
Windows. Specifically, they want to be able to have a generic system in
the kernel which allows the WMI mechanism to be used by userspace,
without the need to patch the kernel for every platform.

This conflicts with the Linux approach, and I've worked with Mario,
Pali, and others to try to bridge this gap with something more
acceptable.

MOF parsing is typically done in userspace, but by doing it in the
kernel, we can do at least some amount of message auditing within the
kernel in a generic way. So long as vendors continue using the same
GUIDs and provide proper MOF data, the kernel wouldn't need to be
changed. New GUIDs require new drivers, which must create the function
ops to get the char device created.

I thought this served as a pragmatic bridge between the two approaches.

This particular driver, doesn't have the benefit of the MOF data. It is
a halfway point intended to eliminate the SMI access to SMBIOS and
replace it with the WMI access, which uses an op region instead of
passing a physical memory pointer to SMM - but doesn't improve on the
message audit of the existing mechanism (but it shouldn't make it any
worse either).

Greg KH Oct. 5, 2017, 6:47 p.m. UTC | #8

On Thu, Oct 05, 2017 at 10:39:25AM -0700, Darren Hart wrote:
> > It does, thanks.  And as we now understand it (I'm guessing it had to be
> > semi-understood in the older wmi drivers already), validating it
> > properly seems to be the key for creating an interface that we "know" to
> > be safe.
> > 
> 
> We don't use the MOF data in any of the existing wmi drivers, because
> they are all oddities which map to kernel managed subsystems (hotkeys,
> LED control, RF Kill switches) rather than what WMI (Windows
> Manageability Interface) was designed for. The intent of these patches
> to enable that management aspect of the platform.
> 
> This is the biggest hurdle for WMI support.
> 
> WMI was designed to bypass the OS, and is used in consumer devices
> intended to run Windows. This leads to an interface that is very vendor
> specific and not consistently broken up into nice functional blocks.
> 
> Vendors would like to use this interface in Linux as it is being used in
> Windows. Specifically, they want to be able to have a generic system in
> the kernel which allows the WMI mechanism to be used by userspace,
> without the need to patch the kernel for every platform.

And how _exactly_ is this interface exposed in Windows?  Is it ad-hoc
with custom kernel drivers written by each vendor?  Or does the OS
provide a "sane" interface for it?

> This conflicts with the Linux approach, and I've worked with Mario,
> Pali, and others to try to bridge this gap with something more
> acceptable.
> 
> MOF parsing is typically done in userspace, but by doing it in the
> kernel, we can do at least some amount of message auditing within the
> kernel in a generic way. So long as vendors continue using the same
> GUIDs and provide proper MOF data, the kernel wouldn't need to be
> changed. New GUIDs require new drivers, which must create the function
> ops to get the char device created.
> 
> I thought this served as a pragmatic bridge between the two approaches.

The code as-is isn't a bridge at all, it's a pass-through tunnel with no
tollbooth.  No parsing is being done that I can see here (if it is,
where exactly was it done?)

> This particular driver, doesn't have the benefit of the MOF data. It is
> a halfway point intended to eliminate the SMI access to SMBIOS and
> replace it with the WMI access, which uses an op region instead of
> passing a physical memory pointer to SMM - but doesn't improve on the
> message audit of the existing mechanism (but it shouldn't make it any
> worse either).

Again, it looks just to be a pass-through, no validation happening at
all, with a random "blob" appended that userspace knows all about, and
the BIOS knows about, but the kernel has no clue.  Given that the kernel
is what is there to protect the BIOS from userspace, that feels really
wrong.

Again, I like my TPM to work, and I don't want a random rootkit exploit
to be able to destroy it :)

thanks,

greg k-h

Limonciello, Mario Oct. 5, 2017, 7:03 p.m. UTC | #9

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 1:47 PM
> To: Darren Hart <[e-mail]>
> Cc: Pali Rohár <[e-mail]>; Limonciello, Mario
> <[e-mail]>; [e-mail]; linux-
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Thu, Oct 05, 2017 at 10:39:25AM -0700, Darren Hart wrote:
> > > It does, thanks.  And as we now understand it (I'm guessing it had to be
> > > semi-understood in the older wmi drivers already), validating it
> > > properly seems to be the key for creating an interface that we "know" to
> > > be safe.
> > >
> >
> > We don't use the MOF data in any of the existing wmi drivers, because
> > they are all oddities which map to kernel managed subsystems (hotkeys,
> > LED control, RF Kill switches) rather than what WMI (Windows
> > Manageability Interface) was designed for. The intent of these patches
> > to enable that management aspect of the platform.
> >
> > This is the biggest hurdle for WMI support.
> >
> > WMI was designed to bypass the OS, and is used in consumer devices
> > intended to run Windows. This leads to an interface that is very vendor
> > specific and not consistently broken up into nice functional blocks.
> >
> > Vendors would like to use this interface in Linux as it is being used in
> > Windows. Specifically, they want to be able to have a generic system in
> > the kernel which allows the WMI mechanism to be used by userspace,
> > without the need to patch the kernel for every platform.
> 
> And how _exactly_ is this interface exposed in Windows?  Is it ad-hoc
> with custom kernel drivers written by each vendor?  Or does the OS
> provide a "sane" interface for it?

On Windows it's a driver-less solution.  Vendors don't do anything other
than provide the MOF (which describes how the data passed to ASL looks).

When Windows boots up, _WDG is parsed, the binary MOF is loaded into
the WMI repository.  The MOF describes how named objects map to GUIDs
which map to ASL.

From Powershell or from any application that uses WMI as admin you can 
look up the root namespace and see all objects.  You can pass calls back
and forth.  There's all sorts of examples of it here:
https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx

Windows doesn't validate the data when it's passed to ASL and back.
It just knows what it looks like, size of the buffer and relays the information.

It's up to firmware to block the crazy stuff that you can put in a buffer.

> 
> > This conflicts with the Linux approach, and I've worked with Mario,
> > Pali, and others to try to bridge this gap with something more
> > acceptable.
> >
> > MOF parsing is typically done in userspace, but by doing it in the
> > kernel, we can do at least some amount of message auditing within the
> > kernel in a generic way. So long as vendors continue using the same
> > GUIDs and provide proper MOF data, the kernel wouldn't need to be
> > changed. New GUIDs require new drivers, which must create the function
> > ops to get the char device created.
> >
> > I thought this served as a pragmatic bridge between the two approaches.
> 
> The code as-is isn't a bridge at all, it's a pass-through tunnel with no
> tollbooth.  No parsing is being done that I can see here (if it is,
> where exactly was it done?)
> 
> > This particular driver, doesn't have the benefit of the MOF data. It is
> > a halfway point intended to eliminate the SMI access to SMBIOS and
> > replace it with the WMI access, which uses an op region instead of
> > passing a physical memory pointer to SMM - but doesn't improve on the
> > message audit of the existing mechanism (but it shouldn't make it any
> > worse either).
> 
> Again, it looks just to be a pass-through, no validation happening at
> all, with a random "blob" appended that userspace knows all about, and
> the BIOS knows about, but the kernel has no clue.  Given that the kernel
> is what is there to protect the BIOS from userspace, that feels really
> wrong.
> 

It's entirely reasonable that Linux can filter more than Windows
can given there ARE vendor specific drivers in the Linux implementation
of WMI.

I'll add some verification of the supported calls on the system being run
on for v5.  Some of the write once only things can be filtered too here.

> Again, I like my TPM to work, and I don't want a random rootkit exploit
> to be able to destroy it :)

I'd like to however point out you can't kill your TPM from this interface.

Greg KH Oct. 5, 2017, 7:09 p.m. UTC | #10

On Thu, Oct 05, 2017 at 07:03:24PM +0000, [e-mail] wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:[e-mail]]
> > Sent: Thursday, October 5, 2017 1:47 PM
> > To: Darren Hart <[e-mail]>
> > Cc: Pali Rohár <[e-mail]>; Limonciello, Mario
> > <[e-mail]>; [e-mail]; linux-
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]; [e-mail]
> > Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when
> > requested by drivers
> > 
> > On Thu, Oct 05, 2017 at 10:39:25AM -0700, Darren Hart wrote:
> > > > It does, thanks.  And as we now understand it (I'm guessing it had to be
> > > > semi-understood in the older wmi drivers already), validating it
> > > > properly seems to be the key for creating an interface that we "know" to
> > > > be safe.
> > > >
> > >
> > > We don't use the MOF data in any of the existing wmi drivers, because
> > > they are all oddities which map to kernel managed subsystems (hotkeys,
> > > LED control, RF Kill switches) rather than what WMI (Windows
> > > Manageability Interface) was designed for. The intent of these patches
> > > to enable that management aspect of the platform.
> > >
> > > This is the biggest hurdle for WMI support.
> > >
> > > WMI was designed to bypass the OS, and is used in consumer devices
> > > intended to run Windows. This leads to an interface that is very vendor
> > > specific and not consistently broken up into nice functional blocks.
> > >
> > > Vendors would like to use this interface in Linux as it is being used in
> > > Windows. Specifically, they want to be able to have a generic system in
> > > the kernel which allows the WMI mechanism to be used by userspace,
> > > without the need to patch the kernel for every platform.
> > 
> > And how _exactly_ is this interface exposed in Windows?  Is it ad-hoc
> > with custom kernel drivers written by each vendor?  Or does the OS
> > provide a "sane" interface for it?
> 
> On Windows it's a driver-less solution.  Vendors don't do anything other
> than provide the MOF (which describes how the data passed to ASL looks).

How do they "provide it"?

> When Windows boots up, _WDG is parsed,

Who parses it, the Windows kernel?

> the binary MOF is loaded into the WMI repository.

Who does the loading?  Where does the "WMI repository" live?

> The MOF describes how named objects map to GUIDs which map to ASL.

So this all lives in kernelspace?

> From Powershell or from any application that uses WMI as admin you can 
> look up the root namespace and see all objects.

And what is the interface that powershell uses to get that information
from the kerenel?

> You can pass calls back
> and forth.  There's all sorts of examples of it here:
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
> 
> Windows doesn't validate the data when it's passed to ASL and back.

How do you know?  Who does the "passing"?  The Windows kernel is just a
blind pipe?  If so, then what does the mappings?

> It just knows what it looks like, size of the buffer and relays the information.

relays from/to what?

> It's up to firmware to block the crazy stuff that you can put in a buffer.

So userspace can pass any blob it wants to the firmware through this
interface and the kernel does not parse anything?  How is that
"protected"?

> > Again, I like my TPM to work, and I don't want a random rootkit exploit
> > to be able to destroy it :)
> 
> I'd like to however point out you can't kill your TPM from this interface.

On _your_ platform, can you guarantee it on any other platform?  :)

And I strongly doubt your BIOS would stand up to a good fuzzer, almost
no firmware can.  Heck, the kernel even has issues, we've been fixing
them for years...

thanks,

greg k-h

Pali Rohár Oct. 5, 2017, 7:32 p.m. UTC | #11

Hi Greg! I would answer some of your question...

On Thursday 05 October 2017 21:09:48 Greg KH wrote:
> On Thu, Oct 05, 2017 at 07:03:24PM +0000, [e-mail] wrote:
> > On Windows it's a driver-less solution.  Vendors don't do anything other
> > than provide the MOF (which describes how the data passed to ASL looks).
> 
> How do they "provide it"?

I described it in my email. Binary MOF is in ACPI buffer returned by
well known ACPI function.

Vendor fills into DSDT ACPI table (part of BIOS firmware) at correct
place binary MOF buffer and ACPI interpreter which is part of kernel can
access it.

> > When Windows boots up, _WDG is parsed,
> 
> Who parses it, the Windows kernel?

Yes, some Windows kernel driver parses ACPI's _WDG and also binary MOF
buffer from ACPI.

> > the binary MOF is loaded into the WMI repository.
> 
> Who does the loading?  Where does the "WMI repository" live?

IIRC it can be loaded by both userspace and kernel. No idea about
permissions, but ACPI/WMI kernel driver loads that binary MOF from ACPI
table.

Via some tools user can loads also its own MOF buffer into "WMI
repository".

> > It's up to firmware to block the crazy stuff that you can put in a buffer.
> 
> So userspace can pass any blob it wants to the firmware through this
> interface and the kernel does not parse anything?  How is that
> "protected"?

In that binary MOF is described C++ style object system and mapping from
those class/objects into ACPI methods. With information how are mapped
object method parameters (e.g. integer, strings, structures...) into
ACPI buffer. And in Windows you can IIRC call those class/objects
defined in MOF and something (I bet Windows kernel driver) translate
class method parameters (integers, strings, ...) into kernel ACPI
buffers and call correct ACPI method. So there is just protection that
method is called with correct "signature".

But IIRC Dell describe in its MOF object system, that there is one class
with just one method which has parameter "String" at specified size --
which maps 1:1 to ACPI buffer.

Limonciello, Mario Oct. 5, 2017, 7:34 p.m. UTC | #12

> -----Original Message-----
> From: Greg KH [mailto:[e-mail]]
> Sent: Thursday, October 5, 2017 2:10 PM
> To: Limonciello, Mario <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Thu, Oct 05, 2017 at 07:03:24PM +0000, [e-mail] wrote:
> > > -----Original Message-----
> > > From: Greg KH [mailto:[e-mail]]
> > > Sent: Thursday, October 5, 2017 1:47 PM
> > > To: Darren Hart <[e-mail]>
> > > Cc: Pali Rohár <[e-mail]>; Limonciello, Mario
> > > <[e-mail]>; [e-mail]; linux-
> > > [e-mail]; [e-mail];
> [e-mail];
> > > [e-mail]; [e-mail]; [e-mail]; [e-mail]
> > > Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices
> when
> > > requested by drivers
> > >
> > > On Thu, Oct 05, 2017 at 10:39:25AM -0700, Darren Hart wrote:
> > > > > It does, thanks.  And as we now understand it (I'm guessing it had to be
> > > > > semi-understood in the older wmi drivers already), validating it
> > > > > properly seems to be the key for creating an interface that we "know" to
> > > > > be safe.
> > > > >
> > > >
> > > > We don't use the MOF data in any of the existing wmi drivers, because
> > > > they are all oddities which map to kernel managed subsystems (hotkeys,
> > > > LED control, RF Kill switches) rather than what WMI (Windows
> > > > Manageability Interface) was designed for. The intent of these patches
> > > > to enable that management aspect of the platform.
> > > >
> > > > This is the biggest hurdle for WMI support.
> > > >
> > > > WMI was designed to bypass the OS, and is used in consumer devices
> > > > intended to run Windows. This leads to an interface that is very vendor
> > > > specific and not consistently broken up into nice functional blocks.
> > > >
> > > > Vendors would like to use this interface in Linux as it is being used in
> > > > Windows. Specifically, they want to be able to have a generic system in
> > > > the kernel which allows the WMI mechanism to be used by userspace,
> > > > without the need to patch the kernel for every platform.
> > >
> > > And how _exactly_ is this interface exposed in Windows?  Is it ad-hoc
> > > with custom kernel drivers written by each vendor?  Or does the OS
> > > provide a "sane" interface for it?
> >
> > On Windows it's a driver-less solution.  Vendors don't do anything other
> > than provide the MOF (which describes how the data passed to ASL looks).
> 
> How do they "provide it"?

In that link I shared you can see there is a GUID that maps to the one that
has the binary MOF.  The MOF is compiled when the BIOS is built using
mofcomp.exe on Windows into a binary format.

> 
> > When Windows boots up, _WDG is parsed,
> 
> Who parses it, the Windows kernel?
> 
Presumably?  Wmiacpi.sys.  I believe it to be user side, but I don't know Windows
source code and where they draw the line.

> > the binary MOF is loaded into the WMI repository.
> 
> Who does the loading?  Where does the "WMI repository" live?

Windows OS does this.  Same wmiacpi.sys I believe.  The WMI repository is a
database that's on disk.  It doesn't just do these things, it contains MOF (managed
object format) for a bunch of stuff.  You can tweak way too many knobs with it.

It's store in "%windir%System32\Wbem\Repository"
There's more info about how it works here
https://technet.microsoft.com/en-us/library/cc753534.aspx

I do have a longer term vision for Linux that CIMOM tools will be able to parse
what the kernel shares in sysfs from the WMI bus and know how to work with 
these character devices.

My patch series is the first part of that, and there are still a lot of other steps.

> 
> > The MOF describes how named objects map to GUIDs which map to ASL.
> 
> So this all lives in kernelspace?
> 
> > From Powershell or from any application that uses WMI as admin you can
> > look up the root namespace and see all objects.
> 
> And what is the interface that powershell uses to get that information
> from the kerenel?

You query the WMI repository for available namespaces.  The stuff in the binary
MOF will have registered under a particular namespace.
When you look at that namespace, you can query classes within it.

Some of those classes will enumerate objects that you can read attributes.
If you're calling a method, you need to fill in the arguments as described in the
MOF.  You can query the namespace to tell what those arguments look like.

So going to my patch series, "dell-wmi-descriptor" would map out to a class
In the namespace.  You can query it to see what the size of the buffer looks
like and what the interface is.

The class that in Linux represents "dell-smbios-wmi" would map to another one.  
You can't enumerate it since It's not a data type, but you can look at what arguments
it takes and if you provide the arguments it expects you can call it and the OS
will pass it to the ASL and return the result.

It's not very different than what I proposed in my patch series.

> 
> > You can pass calls back
> > and forth.  There's all sorts of examples of it here:
> > https://msdn.microsoft.com/en-
> us/library/windows/hardware/dn614028(v=vs.85).aspx
> >
> > Windows doesn't validate the data when it's passed to ASL and back.
> 
> How do you know?  Who does the "passing"?  The Windows kernel is just a
> blind pipe?  If so, then what does the mappings?

The application or cmdlet calling the method passes the data as described
in the MOF which can be queried from the namespace.

Windows doesn't know anything about what that data is, it can't.  All it knows
is the format of the data as described in the MOF.  It knows which ASL method
to call by the GUID in the MOF which matches the GUID in _WDG which contains
the ASL method.

I can't confidently know which parts of that are done by Windows kernel and
which are handled by Windows userspace.  All I know is that from a "consumer"
of this the application or cmdlet runs in userspace. 

> 
> > It just knows what it looks like, size of the buffer and relays the information.
> 
> relays from/to what?

The ASL method.

Let me try to explain better.
_WDG contains a bunch of objects.
Each of those objects has a GUID, instance count, data type, and two letters that
correspond to an ASL method.
One of those objects is binary MOF.

Binary MOF contains a description of namespaces, objects, their formats etc.
It also contains classes that map to GUIDs.

Windows boots up, some process parses _WDG.
It looks at binary MOF, decompiles/parses it.
It maps the classes to GUIDs, the GUIDs to ASL methods.

You call an object of a class, that will then call the ASL method.

Make more sense?
> 
> > It's up to firmware to block the crazy stuff that you can put in a buffer.
> 
> So userspace can pass any blob it wants to the firmware through this
> interface and the kernel does not parse anything?  How is that
> "protected"?
> 
> > > Again, I like my TPM to work, and I don't want a random rootkit exploit
> > > to be able to destroy it :)
> >
> > I'd like to however point out you can't kill your TPM from this interface.
> 
> On _your_ platform, can you guarantee it on any other platform?  :)
> 
> And I strongly doubt your BIOS would stand up to a good fuzzer, almost
> no firmware can.  Heck, the kernel even has issues, we've been fixing
> them for years...
> 
Well that's software for you.  It will always have problems, but

Limonciello, Mario Oct. 5, 2017, 7:39 p.m. UTC | #13

> -----Original Message-----

> From: Pali Rohár [mailto:[e-mail]]

> Sent: Thursday, October 5, 2017 2:33 PM

> To: Greg KH <[e-mail]>

> Cc: Limonciello, Mario <[e-mail]>; [e-mail];

> [e-mail]; [e-mail]; platform-driver-

> [e-mail]; [e-mail]; [e-mail];

> [e-mail]; [e-mail]; [e-mail]

> Subject: Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when

> requested by drivers

> 

> Hi Greg! I would answer some of your question...

> 

> On Thursday 05 October 2017 21:09:48 Greg KH wrote:

> > On Thu, Oct 05, 2017 at 07:03:24PM +0000, [e-mail] wrote:

> > > On Windows it's a driver-less solution.  Vendors don't do anything other

> > > than provide the MOF (which describes how the data passed to ASL looks).

> >

> > How do they "provide it"?

> 

> I described it in my email. Binary MOF is in ACPI buffer returned by

> well known ACPI function.

> 

> Vendor fills into DSDT ACPI table (part of BIOS firmware) at correct

> place binary MOF buffer and ACPI interpreter which is part of kernel can

> access it.

> 

> > > When Windows boots up, _WDG is parsed,

> >

> > Who parses it, the Windows kernel?

> 

> Yes, some Windows kernel driver parses ACPI's _WDG and also binary MOF

> buffer from ACPI.

> 

> > > the binary MOF is loaded into the WMI repository.

> >

> > Who does the loading?  Where does the "WMI repository" live?

> 

> IIRC it can be loaded by both userspace and kernel. No idea about

> permissions, but ACPI/WMI kernel driver loads that binary MOF from ACPI

> table.

> 

> Via some tools user can loads also its own MOF buffer into "WMI

> repository".

> 

> > > It's up to firmware to block the crazy stuff that you can put in a buffer.

> >

> > So userspace can pass any blob it wants to the firmware through this

> > interface and the kernel does not parse anything?  How is that

> > "protected"?

> 

> In that binary MOF is described C++ style object system and mapping from

> those class/objects into ACPI methods. With information how are mapped

> object method parameters (e.g. integer, strings, structures...) into

> ACPI buffer. And in Windows you can IIRC call those class/objects

> defined in MOF and something (I bet Windows kernel driver) translate

> class method parameters (integers, strings, ...) into kernel ACPI

> buffers and call correct ACPI method. So there is just protection that

> method is called with correct "signature".

> 

> But IIRC Dell describe in its MOF object system, that there is one class

> with just one method which has parameter "String" at specified size --

> which maps 1:1 to ACPI buffer.

> 


In the current MOF implementation for Dell SMBIOS calling interface yes similar
to your description.
One class describes data filled with bytes, and then the class for making the 
call will use that as an argument.

class BDat {
    [WmiDataId(1), read, write, Description("data")] uint8 Bytes[WMI_PACKAGE_SIZE];                  };

void DoBFn([in, out, Description("Fn buf")] BDat Data);

Darren Hart Oct. 5, 2017, 8:51 p.m. UTC | #14

On Thu, Oct 05, 2017 at 07:03:24PM +0000, [e-mail] wrote:
> > 
> > And how _exactly_ is this interface exposed in Windows?  Is it ad-hoc
> > with custom kernel drivers written by each vendor?  Or does the OS
> > provide a "sane" interface for it?
> 
> On Windows it's a driver-less solution.  Vendors don't do anything other
> than provide the MOF (which describes how the data passed to ASL looks).
> 
> When Windows boots up, _WDG is parsed, the binary MOF is loaded into
> the WMI repository.  The MOF describes how named objects map to GUIDs
> which map to ASL.
> 
> >From Powershell or from any application that uses WMI as admin you can 
> look up the root namespace and see all objects.  You can pass calls back
> and forth.  There's all sorts of examples of it here:
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
> 

A couple snippets from this link which I believe I've shared previously
that clarify the design intent of the WMI mechanism:

"
ACPI-to-WMI Mapper Goals for Windows Instrumentation:

...

* Allow OEMs to include OEM-specific data blocks, user-mode-callable
  ACPI control methods, and events without requiring any changes to the
  ACPI-to-WMI mapper.

* Allow general-purpose data consumer applications--those written
  without any special knowledge of the data blocks exposed by a
  particular machine--to be able to access and understand the data
  blocks, user-mode-callable ACPI control methods, and events being
  mapped--including those that are OEM specific.

...

The following are not goals for the ACPI-to-WMI mapper:

* To have specific knowledge about any data block that passes through
  the mapper.

* To provide interfaces specifically for SMBIOS data and functions. The
  mapper is an open architecture that is not restricted to SMBIOS data
  and functionality.
"

This model is not consistent with Linux design principles, and Mario's
changes attempt to allow for more kernel oversight by:

* Requiring a driver to be written to bind to any GUID which will expose
  WMI methods to userspace. The goal here is to provide an effective
  whitelist, and to promote vendor participation (they need to send the
  patch, have it reviewed, respond to challenges on the security
  implications, etc.)

* In the future, provide for the MOF parsing within the kernel so Linux
  will have more ability to audit messaging.

Darren Hart Oct. 5, 2017, 8:58 p.m. UTC | #15

On Thu, Oct 05, 2017 at 09:09:48PM +0200, Greg KH wrote:
> On Thu, Oct 05, 2017 at 07:03:24PM +0000, [e-mail] wrote:
...
> > It's up to firmware to block the crazy stuff that you can put in a buffer.
> 
> So userspace can pass any blob it wants to the firmware through this
> interface and the kernel does not parse anything?  How is that
> "protected"?
> 
> > > Again, I like my TPM to work, and I don't want a random rootkit exploit
> > > to be able to destroy it :)
> > 
> > I'd like to however point out you can't kill your TPM from this interface.
> 
> On _your_ platform, can you guarantee it on any other platform?  :)

The dell-smbios-wmi driver won't load on any other platform. No
character device is created for any other platform. When drivers are
written for those other platforms for different WMI GUIDs, we need to
review them.

This driver not having MOF data should be the exception. We'll have more
ability to inspect others. If drivers are submitted that don't look at
the MOF data even through it is present, we should reject them.

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 0357e9b1cfaf..6db1d84999bc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -372,6 +372,7 @@  ACPI WMI DRIVER
 L:	[e-mail]
 S:	Orphan
 F:	drivers/platform/x86/wmi.c
+F:	include/uapi/linux/wmi.h
 
 AD1889 ALSA SOUND DRIVER
 M:	Thibaut Varene <[e-mail]>
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bcb41c1c7f52..5aef052b4aab 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -38,6 +38,7 @@ 
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
+#include <linux/miscdevice.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
@@ -69,6 +70,7 @@  struct wmi_block {
 	struct wmi_device dev;
 	struct list_head list;
 	struct guid_block gblock;
+	struct miscdevice misc_dev;
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
@@ -765,22 +767,80 @@  static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	return 0;
 }
 
+static long wmi_ioctl(struct file *filp, unsigned int cmd,
+		      unsigned long arg)
+{
+	struct wmi_driver *wdriver;
+	struct wmi_block *wblock;
+	const char *driver_name;
+	struct list_head *p;
+	bool found = false;
+
+	if (_IOC_TYPE(cmd) != WMI_IOC)
+		return -ENOTTY;
+
+	driver_name = filp->f_path.dentry->d_iname;
+
+	list_for_each(p, &wmi_block_list) {
+		wblock = list_entry(p, struct wmi_block, list);
+		wdriver = container_of(wblock->dev.dev.driver,
+			struct wmi_driver, driver);
+		if (strcmp(driver_name, wdriver->driver.name) == 0) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found ||
+	    !wdriver->file_operations ||
+	    !wdriver->file_operations->unlocked_ioctl)
+		return -ENODEV;
+
+	/* make sure we're not calling a higher instance */
+	if (_IOC_NR(cmd) > wblock->gblock.instance_count)
+		return -EINVAL;
+
+	return wdriver->file_operations->unlocked_ioctl(filp, cmd, arg);
+}
+
+static const struct file_operations wmi_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= wmi_ioctl,
+};
+
 static int wmi_dev_probe(struct device *dev)
 {
 	struct wmi_block *wblock = dev_to_wblock(dev);
 	struct wmi_driver *wdriver =
 		container_of(dev->driver, struct wmi_driver, driver);
 	int ret = 0;
+	char *buf;
 
 	if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");
 
+	/* driver wants a character device made */
+	if (wdriver->file_operations) {
+		buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+		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);
+		}
+	}
+
 	if (wdriver->probe) {
 		ret = wdriver->probe(dev_to_wdev(dev));
 		if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
 			dev_warn(dev, "failed to disable device\n");
 	}
-
 	return ret;
 }
 
@@ -791,6 +851,11 @@  static int wmi_dev_remove(struct device *dev)
 		container_of(dev->driver, struct wmi_driver, driver);
 	int ret = 0;
 
+	if (wdriver->file_operations) {
+		kfree(wblock->misc_dev.name);
+		misc_deregister(&wblock->misc_dev);
+	}
+
 	if (wdriver->remove)
 		ret = wdriver->remove(dev_to_wdev(dev));
 
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index ddee427e0721..c84db3e8038d 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -18,6 +18,7 @@ 
 
 #include <linux/device.h>
 #include <linux/acpi.h>
+#include <uapi/linux/wmi.h>
 
 struct wmi_device {
 	struct device dev;
@@ -43,6 +44,7 @@  struct wmi_device_id {
 struct wmi_driver {
 	struct device_driver driver;
 	const struct wmi_device_id *id_table;
+	const struct file_operations *file_operations;
 
 	int (*probe)(struct wmi_device *wdev);
 	int (*remove)(struct wmi_device *wdev);
diff --git a/include/uapi/linux/wmi.h b/include/uapi/linux/wmi.h
new file mode 100644
index 000000000000..6a811ead7db8
--- /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*)
+
+#endif