| 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
| ||
|
Message-ID: <CAD584isUWcF-0ZVpQukxPeqoCqxbC8GVLuMJf++oJcRJsqQpAw@mail.gmail.com>
Date: Thu, 2 Nov 2017 11:31:39 +1100
From: "Edward O'Callaghan" <quasisec@...gle.com>
To: Mario Limonciello <mario.limonciello@...l.com>
Cc: dvhart@...radead.org, Andy Shevchenko <andy.shevchenko@...il.com>,
LKML <linux-kernel@...r.kernel.org>,
platform-driver-x86@...r.kernel.org,
Andy Lutomirski <luto@...nel.org>, pali.rohar@...il.com,
rjw@...ysocki.net, Matthew Garrett <mjg59@...gle.com>, hch@....de,
Greg KH <greg@...ah.com>, Alan Cox <gnomes@...rguk.ukuu.org.uk>
Subject: Re: [PATCH v12 14/16] platform/x86: wmi: create userspace interface
for drivers
Reviewed-by: Edward O'Callaghan <quasisec@...gle.com>
On Thu, Nov 2, 2017 at 6:25 AM, Mario Limonciello
<mario.limonciello@...l.com> wrote:
> For WMI operations that are only Set or Query readable and writable sysfs
> attributes created by WMI vendor drivers or the bus driver makes 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 a callback method in the wmi_driver
> the WMI bus driver will create a character device that maps to that
> function. This callback method will be responsible for filtering
> invalid requests and performing the actual call.
>
> That character device will correspond to this path:
> /dev/wmi/$driver
>
> Performing read() on this character device will provide the size
> of the buffer that the character device needs to perform calls.
> This buffer size can be set by vendor drivers through a new symbol
> or when MOF parsing is available by the MOF.
>
> Performing ioctl() on this character device will be interpretd
> by the WMI bus driver. It will perform sanity tests for size of
> data, test them for a valid instance, copy the data from userspace
> and pass iton to the vendor driver to further process and run.
>
> This creates an implicit policy that each driver will only be allowed
> a single character device. If a module matches multiple GUID's,
> the wmi_devices will need to be all handled by the same wmi_driver.
>
> The WMI vendor drivers will be responsible for managing inappropriate
> access to this character device and proper locking on data used by
> it.
>
> When a WMI vendor driver is unloaded the WMI bus driver will clean
> up the character device and any memory allocated for the call.
>
> Signed-off-by: Mario Limonciello <mario.limonciello@...l.com>
> ---
> MAINTAINERS | 1 +
> drivers/platform/x86/wmi.c | 189 ++++++++++++++++++++++++++++++++++++++++++++-
> include/linux/wmi.h | 5 ++
> include/uapi/linux/wmi.h | 26 +++++++
> 4 files changed, 219 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/wmi.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0d7061c05b15..203958d18aec 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -384,6 +384,7 @@ ACPI WMI DRIVER
> L: platform-driver-x86@...r.kernel.org
> S: Orphan
> F: drivers/platform/x86/wmi.c
> +F: include/uapi/linux/wmi.h
>
> AD1889 ALSA SOUND DRIVER
> M: Thibaut Varene <T-Bone@...isc-linux.org>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index bcb41c1c7f52..8c31ed4f0e1b 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -38,12 +38,15 @@
> #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>
> #include <linux/types.h>
> +#include <linux/uaccess.h>
> #include <linux/uuid.h>
> #include <linux/wmi.h>
> +#include <uapi/linux/wmi.h>
>
> ACPI_MODULE_NAME("wmi");
> MODULE_AUTHOR("Carlos Corbacho");
> @@ -69,9 +72,12 @@ struct wmi_block {
> struct wmi_device dev;
> struct list_head list;
> struct guid_block gblock;
> + struct miscdevice char_dev;
> + struct mutex char_mutex;
> struct acpi_device *acpi_device;
> wmi_notify_handler handler;
> void *handler_data;
> + u64 req_buf_size;
>
> bool read_takes_no_args;
> };
> @@ -188,6 +194,25 @@ static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
> /*
> * Exported WMI functions
> */
> +
> +/**
> + * set_required_buffer_size - Sets the buffer size needed for performing IOCTL
> + * @wdev: A wmi bus device from a driver
> + * @instance: Instance index
> + *
> + * Allocates memory needed for buffer, stores the buffer size in that memory
> + */
> +int set_required_buffer_size(struct wmi_device *wdev, u64 length)
> +{
> + struct wmi_block *wblock;
> +
> + wblock = container_of(wdev, struct wmi_block, dev);
> + wblock->req_buf_size = length;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(set_required_buffer_size);
> +
> /**
> * wmi_evaluate_method - Evaluate a WMI method
> * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
> @@ -764,6 +789,111 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
>
> return 0;
> }
> +static int wmi_char_open(struct inode *inode, struct file *filp)
> +{
> + const char *driver_name = filp->f_path.dentry->d_iname;
> + struct wmi_block *wblock = NULL;
> + struct wmi_block *next = NULL;
> +
> + list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
> + if (!wblock->dev.dev.driver)
> + continue;
> + if (strcmp(driver_name, wblock->dev.dev.driver->name) == 0) {
> + filp->private_data = wblock;
> + break;
> + }
> + }
> +
> + if (!filp->private_data)
> + return -ENODEV;
> +
> + return nonseekable_open(inode, filp);
> +}
> +
> +static ssize_t wmi_char_read(struct file *filp, char __user *buffer,
> + size_t length, loff_t *offset)
> +{
> + struct wmi_block *wblock = filp->private_data;
> +
> + return simple_read_from_buffer(buffer, length, offset,
> + &wblock->req_buf_size,
> + sizeof(wblock->req_buf_size));
> +}
> +
> +static long wmi_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
> +{
> + struct wmi_ioctl_buffer __user *input =
> + (struct wmi_ioctl_buffer __user *) arg;
> + struct wmi_block *wblock = filp->private_data;
> + struct wmi_ioctl_buffer *buf = NULL;
> + struct wmi_driver *wdriver = NULL;
> + int ret;
> +
> + if (_IOC_TYPE(cmd) != WMI_IOC)
> + return -ENOTTY;
> +
> + /* make sure we're not calling a higher instance than exists*/
> + if (_IOC_NR(cmd) >= wblock->gblock.instance_count)
> + return -EINVAL;
> +
> + mutex_lock(&wblock->char_mutex);
> + buf = wblock->handler_data;
> + if (get_user(buf->length, &input->length)) {
> + dev_dbg(&wblock->dev.dev, "Read length from user failed\n");
> + ret = -EFAULT;
> + goto out_ioctl;
> + }
> + /* if it's too small, abort */
> + if (buf->length < wblock->req_buf_size) {
> + dev_err(&wblock->dev.dev,
> + "Buffer %lld too small, need at least %lld\n",
> + buf->length, wblock->req_buf_size);
> + ret = -EINVAL;
> + goto out_ioctl;
> + }
> + /* if it's too big, warn, driver will only use what is needed */
> + if (buf->length > wblock->req_buf_size)
> + dev_warn(&wblock->dev.dev,
> + "Buffer %lld is bigger than required %lld\n",
> + buf->length, wblock->req_buf_size);
> +
> + /* copy the structure from userspace */
> + if (copy_from_user(buf, input, wblock->req_buf_size)) {
> + dev_dbg(&wblock->dev.dev, "Copy %llu from user failed\n",
> + wblock->req_buf_size);
> + ret = -EFAULT;
> + goto out_ioctl;
> + }
> +
> + /* let the driver do any filtering and do the call */
> + wdriver = container_of(wblock->dev.dev.driver,
> + struct wmi_driver, driver);
> + if (!try_module_get(wdriver->driver.owner))
> + return -EBUSY;
> + ret = wdriver->filter_callback(&wblock->dev, cmd, buf);
> + module_put(wdriver->driver.owner);
> + if (ret)
> + goto out_ioctl;
> +
> + /* return the result (only up to our internal buffer size) */
> + if (copy_to_user(input, buf, wblock->req_buf_size)) {
> + dev_dbg(&wblock->dev.dev, "Copy %llu to user failed\n",
> + wblock->req_buf_size);
> + ret = -EFAULT;
> + }
> +
> +out_ioctl:
> + mutex_unlock(&wblock->char_mutex);
> + return ret;
> +}
> +
> +static const struct file_operations wmi_fops = {
> + .owner = THIS_MODULE,
> + .read = wmi_char_read,
> + .open = wmi_char_open,
> + .unlocked_ioctl = wmi_ioctl,
> + .compat_ioctl = wmi_ioctl,
> +};
>
> static int wmi_dev_probe(struct device *dev)
> {
> @@ -771,16 +901,63 @@ static int wmi_dev_probe(struct device *dev)
> struct wmi_driver *wdriver =
> container_of(dev->driver, struct wmi_driver, driver);
> int ret = 0;
> + int count;
> + char *buf;
>
> if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
> dev_warn(dev, "failed to enable device -- probing anyway\n");
>
> 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");
> + if (ret != 0)
> + goto probe_failure;
> }
>
> + /* driver wants a character device made */
> + if (wdriver->filter_callback) {
> + /* check that required buffer size declared by driver or MOF */
> + if (!wblock->req_buf_size) {
> + dev_err(&wblock->dev.dev,
> + "Required buffer size not set\n");
> + ret = -EINVAL;
> + goto probe_failure;
> + }
> +
> + count = get_order(wblock->req_buf_size);
> + wblock->handler_data = (void *)__get_free_pages(GFP_KERNEL,
> + count);
> + if (!wblock->handler_data) {
> + ret = -ENOMEM;
> + goto probe_failure;
> + }
> +
> + buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
> + if (!buf) {
> + ret = -ENOMEM;
> + goto probe_string_failure;
> + }
> + sprintf(buf, "wmi/%s", wdriver->driver.name);
> + wblock->char_dev.minor = MISC_DYNAMIC_MINOR;
> + wblock->char_dev.name = buf;
> + wblock->char_dev.fops = &wmi_fops;
> + wblock->char_dev.mode = 0444;
> + ret = misc_register(&wblock->char_dev);
> + if (ret) {
> + dev_warn(dev, "failed to register char dev: %d", ret);
> + ret = -ENOMEM;
> + goto probe_misc_failure;
> + }
> + }
> +
> + return 0;
> +
> +probe_misc_failure:
> + kfree(buf);
> +probe_string_failure:
> + kfree(wblock->handler_data);
> +probe_failure:
> + if (ACPI_FAILURE(wmi_method_enable(wblock, 0)))
> + dev_warn(dev, "failed to disable device\n");
> return ret;
> }
>
> @@ -791,6 +968,13 @@ static int wmi_dev_remove(struct device *dev)
> container_of(dev->driver, struct wmi_driver, driver);
> int ret = 0;
>
> + if (wdriver->filter_callback) {
> + misc_deregister(&wblock->char_dev);
> + kfree(wblock->char_dev.name);
> + free_pages((unsigned long)wblock->handler_data,
> + get_order(wblock->req_buf_size));
> + }
> +
> if (wdriver->remove)
> ret = wdriver->remove(dev_to_wdev(dev));
>
> @@ -847,6 +1031,7 @@ static int wmi_create_device(struct device *wmi_bus_dev,
>
> if (gblock->flags & ACPI_WMI_METHOD) {
> wblock->dev.dev.type = &wmi_type_method;
> + mutex_init(&wblock->char_mutex);
> goto out_init;
> }
>
> diff --git a/include/linux/wmi.h b/include/linux/wmi.h
> index ddee427e0721..4757cb5077e5 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;
> @@ -36,6 +37,8 @@ extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
> extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
> u8 instance);
>
> +extern int set_required_buffer_size(struct wmi_device *wdev, u64 length);
> +
> struct wmi_device_id {
> const char *guid_string;
> };
> @@ -47,6 +50,8 @@ struct wmi_driver {
> int (*probe)(struct wmi_device *wdev);
> int (*remove)(struct wmi_device *wdev);
> void (*notify)(struct wmi_device *device, union acpi_object *data);
> + long (*filter_callback)(struct wmi_device *wdev, unsigned int cmd,
> + struct wmi_ioctl_buffer *arg);
> };
>
> extern int __must_check __wmi_driver_register(struct wmi_driver *driver,
> diff --git a/include/uapi/linux/wmi.h b/include/uapi/linux/wmi.h
> new file mode 100644
> index 000000000000..7e52350ac9b3
> --- /dev/null
> +++ b/include/uapi/linux/wmi.h
> @@ -0,0 +1,26 @@
> +/*
> + * User API methods for ACPI-WMI mapping driver
> + *
> + * Copyright (C) 2017 Dell, Inc.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _UAPI_LINUX_WMI_H
> +#define _UAPI_LINUX_WMI_H
> +
> +#include <linux/types.h>
> +
> +/* WMI bus will filter all WMI vendor driver requests through this IOC */
> +#define WMI_IOC 'W'
> +
> +/* All ioctl requests through WMI should declare their size followed by
> + * relevant data objects
> + */
> +struct wmi_ioctl_buffer {
> + __u64 length;
> + __u8 data[];
> +};
> +
> +#endif
> --
> 2.14.1
>
Powered by blists - more mailing lists