[PATCH v7 00/15] Introduce support for Dell SMBIOS over WMI

[PATCH v7 00/15] Introduce support for Dell SMBIOS over WMI

[Mario Limonciello]

The existing way that the dell-smbios helper module and associated
other drivers (dell-laptop, dell-wmi) communicate with the platform
really isn't secure.  It requires creating a buffer in physical
DMA32 memory space and passing that to the platform via SMM.

Since the platform got a physical memory pointer, you've just got
to trust that the platform has only modified (and accessed) memory
within that buffer.

Dell Platform designers recognize this security risk and offer a
safer way to communicate with the platform over ACPI.  This is
in turn exposed via a WMI interface to the OS.

When communicating over WMI-ACPI the communication doesn't occur
with physical memory pointers.  When the ASL is invoked, the fixed
length ACPI buffer is copied to a small operating region.  The ASL
will invoke the SMI, and SMM will only have access to this operating
region.  When the ASL returns the buffer is copied back for the OS
to process.

This method of communication should also deprecate the usage of the
dcdbas kernel module and software dependent upon it's interface.
Instead offer a character device interface for communicating with this
ASL method to allow userspace to use instead.

To faciliate that this patch series introduces a generic way for WMI
drivers to be able to create discoverable character devices with
a predictable IOCTL interface through the WMI bus when desired.
Requiring WMI drivers to explicitly ask for this functionality will
act as an effective vendor whitelist to character device creation.

Some of this work is the basis for what will be a proper interpreter
of MOF in the kernel and controls for what drivers will be able to
do with that MOF.

NOTE: This patch series is intended to go on top of platform-drivers-x86
linux-next.

For convenience the entire series including those is also available here:
https://github.com/superm1/linux/tree/wmi-smbios
changes between v6 and v7:
 * Use deferred probing in any function results needed from 
   dell-wmi-descriptor
 * Protect against a list entry disappearing when running an ioctl from
   WMI bus
 * Move ioctl uapi declaration into a common header file for all WMI
   drivers.
 * New patch: create required_buffer_size for method type WMI objects 
   in WMI core rather than vendor driver.
 * WSMT patch: Add comment explaining WSMT
 * Filtering patch: Add comments explaining what I can about blacklists.
 * WMI, dell-smbios-wmi: Add back in compat ioctl, it is needed. 
   - My previous test was erroneous.
   - I can't predict if other vendor drivers will use same ioctl for both,
     so add a stub for unlocked or compat.
   - Set up the dell-smbios-wmi driver to use same function for both
     though.
 * dell-laptop, dell-wmi: allocate less memory for buffer, page no longer
   needed
 * dell-laptop make the changes (hopefully) more amenable to pali style
   wise.
 * read SMM cmd address in dell-smbios-smm rather than dell-smbios
 * shuffle structure definitions for this
changes between v5 and v6:
 * Adjust Kconfig for dell-smbios to not depend on anything.
   - SMM and WMI drivers will both select dell-smbios
   - Technically the module can run on it's own (it's just not useful)
 * Add a new tokens sysfs interface
 * Rework blacklisting into easily expandable structures
 * Lock modules during ioctl call from WMI bus
 * drop references to compat ioctl in both WMI and dell-smbios-wmi
   drivers. (Made sure ioctl worked in both 32 and 64 userspace though)
 * dell-smbios-wmi 
   - s/buffer_size/req_buf_size/ to make it clearer that userspace
     doesn't get to set this, it's set internally.
   - read just buffer length before reading whole structure from 
     userspace
   - if buffer length is too big, show a warning
   - I tried to rename argument for unlocked_ioctl, but this caused
     problems in matching initialization lists, so still casting.
   - Add comments clarifying everything happening in ioctl
changes between v4 and v5:
 * Remove Andy's S suggested by in sysfs tokens patch
 * Make some output in dell-wmi-descriptor debug only
 * Adjust various Kconfig dependencies as recommended by Darren
 * Drop patch to set dell-smbios to default on ACPI_WMI,
   it's not needed after the Kconfig dependencies rework
 * Move WSMT check patch to after WMI driver is introduced.
 * Make common smbios call return value int as there could be
   errors now with drivers not being loaded.
 * Make SMBIOS call methods for all drivers return status
 * Reorder patches 2 and 4.
 * Don't export symbols for calling functions on dispatchers
 * wmi patch:
   - use sprintf instead of strcpy
   - remove needless bool for tracking found
   - adjust logic to look for instance_count - 1, it's zero
     based not 1 based.
   - Pass a callback to unlocked_ioctl instead of full file
     operations object
   - ioctl: Don't fail on no bound WMI driver
   - Add missing header for uapi
   - Make helper macros include data types
   - add compat ioctl
 * dell-smbios:
   - Add filtering functionality for SMBIOS calling interface
   - Use dev_dbg rather than pr_debug where possible
 * dell-smbios-wmi:
   - test for handle on b1 table
   - correct misc flags comment
   - drop access checks
   - use dev_dbg instead of pr_* calls
   - use filtering functionality
   - add mutexes around list add/remove
   - switch from get_first_device to get_first_priv and inline
   - add mutex locking to prevent unloading mid-call.
   - update to new ioctl passing
   - fix userspace uapi to use __u32 instead of u32
   - Don't use a header file for internal only use
   - make sure it works with compat ioctl
 * dell-laptop: make dell_smbios_send_request local function
   for boilerplate calls.
 * ioctl patch
   - Change API to have a simpler structure to pass back and
     forth
   - Rename header file
   - Export to sysfs properly
   - Add the size of the length variable into the requested
     buffersize to sysfs, do math in the driver when copying
     data around.
changes between v3 and v4:
 * Make Dell WMI notifications driver fail notifications fail
   when WMI descriptor driver is unavailable.
 * Add a patch to check for Dell OEM string to stop dell-smbios 
   module from being loaded in unsupported systems manually.
 * Split Dell WMI descriptor into it's own driver for others to
   query.
 * Test the misc BIOS flags table to decide whether to run in WMI
   or SMI mode.
 * s/dell-wmi-smbios/dell-smbios/ in a few patch titles
 * Add missing Suggested-by to a patch from v2 (Sorry Andy S!)
 * Adjust cleanup order of wmi character device.
 * Fix a remaining reference to /dev/wmi-$driver
 * Use get_order to get size for pages
 * Split up dell-smbios into 3 drivers:
   dell-smbios
   -> dell-smbios-smm
   -> dell-smbios-wmi
   If either of the two dispatcher drivers is unloaded but
   the other still works, gracefully fall back to that driver
 * Remove unneded open and release on file operations in WMI
   driver
 * Switch to misc character device in WMI bus.
 * Query the size of the calling interface buffer from WMI
   descriptor driver.
 * Require userspace to use this information to build an
   appropriately sized buffer.
 * Verify the size of buffer passed from userspace matches
   expectation of buffer processed in kernel.
 * Add more documentation about the IOCTL API.
 * Add in a recommendation from Andy S about outputing tokens
 * Add a patch to test for non-functional SMM due to WSMT
 * Define macros for IOCTL's through the WMI bus.
 * Pass all IOCTL calls for WMI drivers through WMI bus
   WMI bus will guarantee instance count matches and prevent
   creating too many more IOCTLs.
changes between v2 and v3:
 * Drop patches 1-7, they're in Darren's review tree now
 * Add missing newline on new Documentation
 * Add Reviewed by from Edward O'Callaghan
 * Adjust path of character device from /dev/wmi-$driver to
   /dev/wmi/$driver
 * Store wmi_device pointer rather than using a boolean has_wmi
   to indicate driver is running in WMI mode
 * Don't guard free_page from freeing NULL (this is OK)
 * New patch: add wmidev_evaluate_method to wmi bus as recommended
   by Andy L
 * Adjust ACPI-WMI interface for this patch change ^
 * Add back in sysfs token patch, drop 2nd and 3rd ioctls per discussion
   on mailing list.
 * On sysfs interface: adjust the delimiter to be tabs
 * Drop the rename dell-smbios -> dell-wmi-smbios patch
 * Remove/move some unnecessary tests for CONFIG_DCDBAS
 * Reword s/platform/SMM/ in the WMI-ACPI patch.
 * Update Kconfig to recommend using CONFIG_DCDBAS on old machines.
 * Allocate buffer to the same pointer regardless of the struct 
   assigned to it.  Keep track of the buffer size for cleaning up.
 * Explain policy around character device creation in commit message
changes between v1 and v2:
 * Introduce another patch to sort the includes in wmi.c
 * Introduce another patch to cleanup dell_wmi_check_descriptor_buffer
   checks.
 * Add a commit message to the pr_fmt commit
 * Introduce includes to wmi.c in proper location
 * Add Reviewed-by to relevant patches from Pali
 * Make the WMI introduction patch fallback to legacy SMI
   if compiled with CONFIG_DCDBAS
 * Separate format of WMI and SMI buffers.  WMI buffer supports more
   arguments and data.
 * Adjust the rename patch for changes to fallback
 * Drop sysfs token creation patch
 * Adjust WMI descriptor check patch for changes to fallback
 * introduce another patch to remove needless includes in dell-smbios.c
 * Add token ioctl interface to character device.
   - Can query number of tokens
   - Can query values in all tokens
 * Expose format of all buffers and IOCTL commands to uapi header
 * Drop the read interface from character device.  It doesn't make
   sense with multiple different ioctl methods.
 * Default WMI interface to 32k (This would normally be queried via
   MOF, but that's not possible yet)
 * Create separate buffers for WMI and SMI.  If WMI is available,
   free the SMI buffer.
 * Reorder patches so all fixups come first in the series.

Mario Limonciello (15):
  platform/x86: wmi: Add new method wmidev_evaluate_method
  platform/x86: dell-wmi: increase severity of some failures
  platform/x86: dell-wmi: clean up wmi descriptor check
  platform/x86: dell-wmi: allow 32k return size in the descriptor
  platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own
    driver
  platform/x86: wmi: Don't allow drivers to get each other's GUIDs
  platform/x86: dell-smbios: only run if proper oem string is detected
  platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens
  platform/x86: dell-smbios: Introduce dispatcher for SMM calls
  platform/x86: dell-smbios: add filtering capability for requests
  platform/x86: dell-smbios-wmi: Add new WMI dispatcher driver
  platform/x86: dell-smbios-smm: test for WSMT
  platform/x86: wmi: Add sysfs attribute for required_buffer_size
  platform/x86: wmi: create userspace interface for drivers
  platform/x86: dell-smbios-wmi: introduce userspace interface

 Documentation/ABI/testing/dell-smbios-wmi          |  41 ++
 .../ABI/testing/sysfs-platform-dell-smbios         |  20 +
 MAINTAINERS                                        |  25 ++
 drivers/platform/x86/Kconfig                       |  34 +-
 drivers/platform/x86/Makefile                      |   3 +
 drivers/platform/x86/dell-laptop.c                 | 271 +++++--------
 drivers/platform/x86/dell-smbios-smm.c             | 196 +++++++++
 drivers/platform/x86/dell-smbios-wmi.c             | 288 ++++++++++++++
 drivers/platform/x86/dell-smbios.c                 | 441 ++++++++++++++++++---
 drivers/platform/x86/dell-smbios.h                 |  32 +-
 drivers/platform/x86/dell-wmi-descriptor.c         | 162 ++++++++
 drivers/platform/x86/dell-wmi-descriptor.h         |  18 +
 drivers/platform/x86/dell-wmi.c                    |  91 +----
 drivers/platform/x86/wmi.c                         | 191 ++++++++-
 include/linux/wmi.h                                |  17 +-
 include/uapi/linux/wmi.h                           |  52 +++
 16 files changed, 1544 insertions(+), 338 deletions(-)
 create mode 100644 Documentation/ABI/testing/dell-smbios-wmi
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios
 create mode 100644 drivers/platform/x86/dell-smbios-smm.c
 create mode 100644 drivers/platform/x86/dell-smbios-wmi.c
 create mode 100644 drivers/platform/x86/dell-wmi-descriptor.c
 create mode 100644 drivers/platform/x86/dell-wmi-descriptor.h
 create mode 100644 include/uapi/linux/wmi.h

-- 
2.14.1

[PATCH v7 01/15] platform/x86: wmi: Add new method wmidev_evaluate_method

[Mario Limonciello]

Drivers properly using the wmibus can pass their wmi_device
pointer rather than the GUID back to the WMI bus to evaluate
the proper methods.

Any "new" drivers added that use the WMI bus should use this
rather than the old wmi_evaluate_method that would take the
GUID.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/wmi.c | 28 ++++++++++++++++++++++++----
 include/linux/wmi.h        |  6 ++++++
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 7a05843aff19..4d73a87c2ddf 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -200,6 +200,28 @@ static acpi_status wmi_method_enable(struct wmi_block *wblock, int enable)
  */
 acpi_status wmi_evaluate_method(const char *guid_string, u8 instance,
 u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
+{
+	struct wmi_block *wblock = NULL;
+
+	if (!find_guid(guid_string, &wblock))
+		return AE_ERROR;
+	return wmidev_evaluate_method(&wblock->dev, instance, method_id,
+				      in, out);
+}
+EXPORT_SYMBOL_GPL(wmi_evaluate_method);
+
+/**
+ * wmidev_evaluate_method - Evaluate a WMI method
+ * @wdev: A wmi bus device from a driver
+ * @instance: Instance index
+ * @method_id: Method ID to call
+ * &in: Buffer containing input for the method call
+ * &out: Empty buffer to return the method results
+ *
+ * Call an ACPI-WMI method
+ */
+acpi_status wmidev_evaluate_method(struct wmi_device *wdev, u8 instance,
+	u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
 {
 	struct guid_block *block = NULL;
 	struct wmi_block *wblock = NULL;
@@ -209,9 +231,7 @@ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
 	union acpi_object params[3];
 	char method[5] = "WM";
 
-	if (!find_guid(guid_string, &wblock))
-		return AE_ERROR;
-
+	wblock = container_of(wdev, struct wmi_block, dev);
 	block = &wblock->gblock;
 	handle = wblock->acpi_device->handle;
 
@@ -246,7 +266,7 @@ u32 method_id, const struct acpi_buffer *in, struct acpi_buffer *out)
 
 	return status;
 }
-EXPORT_SYMBOL_GPL(wmi_evaluate_method);
+EXPORT_SYMBOL_GPL(wmidev_evaluate_method);
 
 static acpi_status __query_block(struct wmi_block *wblock, u8 instance,
 				 struct acpi_buffer *out)
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index cd0d7734dc49..2cd10c3b89e9 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -26,6 +26,12 @@ struct wmi_device {
 	bool setable;
 };
 
+/* evaluate the ACPI method associated with this device */
+extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
+					  u8 instance, u32 method_id,
+					  const struct acpi_buffer *in,
+					  struct acpi_buffer *out);
+
 /* Caller must kfree the result. */
 extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
 					     u8 instance);
-- 
2.14.1

[PATCH v7 02/15] platform/x86: dell-wmi: increase severity of some failures

[Mario Limonciello]

There is a lot of error checking in place for the format of the WMI
descriptor buffer, but some of the potentially raised issues should
be considered critical failures.

If the buffer size or header don't match, this is a good indication
that the buffer format changed in a way that the rest of the data
should not be relied upon.

For the remaining data set vectors, continue to notate a warning
in undefined results, but as those are fields that the descriptor
intended to refer to other applications, don't fail if they're new
values.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-wmi.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1fbef560ca67..2cfaaa8faf0a 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -657,17 +657,18 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
 		dev_err(&wdev->dev,
 			"Dell descriptor buffer has invalid length (%d)\n",
 			obj->buffer.length);
-		if (obj->buffer.length < 16) {
-			ret = -EINVAL;
-			goto out;
-		}
+		ret = -EINVAL;
+		goto out;
 	}
 
 	buffer = (u32 *)obj->buffer.pointer;
 
-	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
+	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720) {
+		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
 			8, buffer);
+		ret = -EINVAL;
+		goto out;
+	}
 
 	if (buffer[2] != 0 && buffer[2] != 1)
 		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
-- 
2.14.1

[PATCH v7 03/15] platform/x86: dell-wmi: clean up wmi descriptor check

[Mario Limonciello]

Some cases the wrong type was used for errors and checks can be
done more cleanly.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Reviewed-by: Edward O'Callaghan <quasisec@google.com>
Suggested-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/platform/x86/dell-wmi.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 2cfaaa8faf0a..ece2fe341f01 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -663,19 +663,19 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
 
 	buffer = (u32 *)obj->buffer.pointer;
 
-	if (buffer[0] != 0x4C4C4544 && buffer[1] != 0x494D5720) {
-		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
-			8, buffer);
+	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
+		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+			buffer);
 		ret = -EINVAL;
 		goto out;
 	}
 
 	if (buffer[2] != 0 && buffer[2] != 1)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
 			buffer[2]);
 
 	if (buffer[3] != 4096)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
 			buffer[3]);
 
 	priv->interface_version = buffer[2];
-- 
2.14.1

[PATCH v7 04/15] platform/x86: dell-wmi: allow 32k return size in the descriptor

[Mario Limonciello]

Some platforms this year will be adopting 32k WMI buffer, so don't
complain when encountering those.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-wmi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index ece2fe341f01..c8c7f4f9326c 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -624,7 +624,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
  * Vendor Signature          0       4    "DELL"
  * Object Signature          4       4    " WMI"
  * WMI Interface Version     8       4    <version>
- * WMI buffer length        12       4    4096
+ * WMI buffer length        12       4    4096 or 32768
  */
 static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
 {
@@ -674,7 +674,7 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
 		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
 			buffer[2]);
 
-	if (buffer[3] != 4096)
+	if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)
 		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
 			buffer[3]);
 
-- 
2.14.1

[PATCH v7 05/15] platform/x86: dell-wmi-descriptor: split WMI descriptor into it's own driver

[Mario Limonciello]

All communication on individual GUIDs should occur in separate drivers.
Allowing a driver to communicate with the bus to another GUID is just
a hack that discourages drivers to adopt the bus model.

The information found from the WMI descriptor driver is now exported
for use by other drivers.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 MAINTAINERS                                |   5 +
 drivers/platform/x86/Kconfig               |   5 +
 drivers/platform/x86/Makefile              |   1 +
 drivers/platform/x86/dell-wmi-descriptor.c | 162 +++++++++++++++++++++++++++++
 drivers/platform/x86/dell-wmi-descriptor.h |  18 ++++
 drivers/platform/x86/dell-wmi.c            |  81 +--------------
 6 files changed, 194 insertions(+), 78 deletions(-)
 create mode 100644 drivers/platform/x86/dell-wmi-descriptor.c
 create mode 100644 drivers/platform/x86/dell-wmi-descriptor.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2347e36588dc..f4cf35950b08 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4013,6 +4013,11 @@ M:	Pali Rohár <pali.rohar@gmail.com>
 S:	Maintained
 F:	drivers/platform/x86/dell-wmi.c
 
+DELL WMI DESCRIPTOR DRIVER
+M:	Mario Limonciello <mario.limonciello@dell.com>
+S:	Maintained
+F:	drivers/platform/x86/dell-wmi-descriptor.c
+
 DELTA ST MEDIA DRIVER
 M:	Hugues Fruchet <hugues.fruchet@st.com>
 L:	linux-media@vger.kernel.org
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1f7959ff055c..7722923c968c 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -121,6 +121,7 @@ config DELL_WMI
 	depends on DMI
 	depends on INPUT
 	depends on ACPI_VIDEO || ACPI_VIDEO = n
+	select DELL_WMI_DESCRIPTOR
 	select DELL_SMBIOS
 	select INPUT_SPARSEKMAP
 	---help---
@@ -129,6 +130,10 @@ config DELL_WMI
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-wmi.
 
+config DELL_WMI_DESCRIPTOR
+	tristate
+	depends on ACPI_WMI
+
 config DELL_WMI_AIO
 	tristate "WMI Hotkeys for Dell All-In-One series"
 	depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 2b315d0df3b7..8636f5d3424f 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
 obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
 obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
+obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
 obj-$(CONFIG_DELL_WMI_AIO)	+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED)	+= dell-wmi-led.o
 obj-$(CONFIG_DELL_SMO8800)	+= dell-smo8800.o
diff --git a/drivers/platform/x86/dell-wmi-descriptor.c b/drivers/platform/x86/dell-wmi-descriptor.c
new file mode 100644
index 000000000000..72e317cf0365
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-descriptor.c
@@ -0,0 +1,162 @@
+/*
+ * Dell WMI descriptor driver
+ *
+ * Copyright (C) 2017 Dell Inc. All Rights Reserved.
+ *
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/acpi.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/wmi.h>
+#include "dell-wmi-descriptor.h"
+
+#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
+
+struct descriptor_priv {
+	struct list_head list;
+	u32 interface_version;
+	u32 size;
+};
+static LIST_HEAD(wmi_list);
+
+bool dell_wmi_get_interface_version(u32 *version)
+{
+	struct descriptor_priv *priv;
+
+	priv = list_first_entry_or_null(&wmi_list,
+					struct descriptor_priv,
+					list);
+	if (!priv)
+		return false;
+	*version = priv->interface_version;
+	return true;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_get_interface_version);
+
+bool dell_wmi_get_size(u32 *size)
+{
+	struct descriptor_priv *priv;
+
+	priv = list_first_entry_or_null(&wmi_list,
+					struct descriptor_priv,
+					list);
+	if (!priv)
+		return false;
+	*size = priv->size;
+	return true;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_get_size);
+
+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ *       Name             Offset  Length  Value
+ * Vendor Signature          0       4    "DELL"
+ * Object Signature          4       4    " WMI"
+ * WMI Interface Version     8       4    <version>
+ * WMI buffer length        12       4    4096 or 32768
+ */
+static int dell_wmi_descriptor_probe(struct wmi_device *wdev)
+{
+	union acpi_object *obj = NULL;
+	struct descriptor_priv *priv;
+	u32 *buffer;
+	int ret;
+
+	obj = wmidev_block_query(wdev, 0);
+	if (!obj) {
+		dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
+		ret = -EIO;
+		goto out;
+	}
+
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
+		ret = -EINVAL;
+		goto out;
+	}
+
+	/* Although it's not technically a failure, this would lead to
+	 * unexpected behavior
+	 */
+	if (obj->buffer.length != 128) {
+		dev_err(&wdev->dev,
+			"Dell descriptor buffer has unexpected length (%d)\n",
+			obj->buffer.length);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buffer = (u32 *)obj->buffer.pointer;
+
+	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
+		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+			buffer);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	if (buffer[2] != 0 && buffer[2] != 1)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
+			buffer[2]);
+
+	if (buffer[3] != 4096 && buffer[3] != 32768)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has unexpected buffer length (%u)\n",
+			buffer[3]);
+
+	priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv),
+	GFP_KERNEL);
+
+	priv->interface_version = buffer[2];
+	priv->size = buffer[3];
+	ret = 0;
+	dev_set_drvdata(&wdev->dev, priv);
+	list_add_tail(&priv->list, &wmi_list);
+
+	dev_dbg(&wdev->dev, "Detected Dell WMI interface version %u and buffer size %u\n",
+		priv->interface_version, priv->size);
+
+out:
+	kfree(obj);
+	return ret;
+}
+
+static int dell_wmi_descriptor_remove(struct wmi_device *wdev)
+{
+	struct descriptor_priv *priv = dev_get_drvdata(&wdev->dev);
+
+	list_del(&priv->list);
+	return 0;
+}
+
+static const struct wmi_device_id dell_wmi_descriptor_id_table[] = {
+	{ .guid_string = DELL_WMI_DESCRIPTOR_GUID },
+	{ },
+};
+
+static struct wmi_driver dell_wmi_descriptor_driver = {
+	.driver = {
+		.name = "dell-wmi-descriptor",
+	},
+	.probe = dell_wmi_descriptor_probe,
+	.remove = dell_wmi_descriptor_remove,
+	.id_table = dell_wmi_descriptor_id_table,
+};
+
+module_wmi_driver(dell_wmi_descriptor_driver);
+
+MODULE_ALIAS("wmi:" DELL_WMI_DESCRIPTOR_GUID);
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Dell WMI descriptor driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-wmi-descriptor.h b/drivers/platform/x86/dell-wmi-descriptor.h
new file mode 100644
index 000000000000..3e652c6da034
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-descriptor.h
@@ -0,0 +1,18 @@
+/*
+ *
+ *  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 _DELL_WMI_DESCRIPTOR_H_
+#define _DELL_WMI_DESCRIPTOR_H_
+
+#include <linux/wmi.h>
+
+bool dell_wmi_get_interface_version(u32 *version);
+bool dell_wmi_get_size(u32 *size);
+
+#endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index c8c7f4f9326c..b74d328c9da2 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -39,6 +39,7 @@
 #include <linux/wmi.h>
 #include <acpi/video.h>
 #include "dell-smbios.h"
+#include "dell-wmi-descriptor.h"
 
 MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
 MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
@@ -46,7 +47,6 @@ MODULE_DESCRIPTION("Dell laptop WMI hotkeys driver");
 MODULE_LICENSE("GPL");
 
 #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
-#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 static bool wmi_requires_smbios_request;
 
@@ -617,79 +617,6 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
 	input_unregister_device(priv->input_dev);
 }
 
-/*
- * Descriptor buffer is 128 byte long and contains:
- *
- *       Name             Offset  Length  Value
- * Vendor Signature          0       4    "DELL"
- * Object Signature          4       4    " WMI"
- * WMI Interface Version     8       4    <version>
- * WMI buffer length        12       4    4096 or 32768
- */
-static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
-{
-	struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
-	union acpi_object *obj = NULL;
-	struct wmi_device *desc_dev;
-	u32 *buffer;
-	int ret;
-
-	desc_dev = wmidev_get_other_guid(wdev, DELL_DESCRIPTOR_GUID);
-	if (!desc_dev) {
-		dev_err(&wdev->dev, "Dell WMI descriptor does not exist\n");
-		return -ENODEV;
-	}
-
-	obj = wmidev_block_query(desc_dev, 0);
-	if (!obj) {
-		dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
-		ret = -EIO;
-		goto out;
-	}
-
-	if (obj->type != ACPI_TYPE_BUFFER) {
-		dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (obj->buffer.length != 128) {
-		dev_err(&wdev->dev,
-			"Dell descriptor buffer has invalid length (%d)\n",
-			obj->buffer.length);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	buffer = (u32 *)obj->buffer.pointer;
-
-	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
-		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
-			buffer);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (buffer[2] != 0 && buffer[2] != 1)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
-			buffer[2]);
-
-	if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
-			buffer[3]);
-
-	priv->interface_version = buffer[2];
-	ret = 0;
-
-	dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
-		priv->interface_version);
-
-out:
-	kfree(obj);
-	put_device(&desc_dev->dev);
-	return ret;
-}
-
 /*
  * According to Dell SMBIOS documentation:
  *
@@ -725,7 +652,6 @@ static int dell_wmi_events_set_enabled(bool enable)
 static int dell_wmi_probe(struct wmi_device *wdev)
 {
 	struct dell_wmi_priv *priv;
-	int err;
 
 	priv = devm_kzalloc(
 		&wdev->dev, sizeof(struct dell_wmi_priv), GFP_KERNEL);
@@ -733,9 +659,8 @@ static int dell_wmi_probe(struct wmi_device *wdev)
 		return -ENOMEM;
 	dev_set_drvdata(&wdev->dev, priv);
 
-	err = dell_wmi_check_descriptor_buffer(wdev);
-	if (err)
-		return err;
+	if (!dell_wmi_get_interface_version(&priv->interface_version))
+		return -EPROBE_DEFER;
 
 	return dell_wmi_input_setup(wdev);
 }
-- 
2.14.1

[PATCH v7 06/15] platform/x86: wmi: Don't allow drivers to get each other's GUIDs

[Mario Limonciello]

The only driver using this was dell-wmi, and it really was a hack.
The driver was getting a data attribute from another driver and this
type of action should not be encouraged.

Rather drivers that need to interact with one another should pass
data back and forth via exported functions.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/wmi.c | 17 -----------------
 include/linux/wmi.h        |  4 ----
 2 files changed, 21 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4d73a87c2ddf..bcb41c1c7f52 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -368,23 +368,6 @@ union acpi_object *wmidev_block_query(struct wmi_device *wdev, u8 instance)
 }
 EXPORT_SYMBOL_GPL(wmidev_block_query);
 
-struct wmi_device *wmidev_get_other_guid(struct wmi_device *wdev,
-					 const char *guid_string)
-{
-	struct wmi_block *this_wb = container_of(wdev, struct wmi_block, dev);
-	struct wmi_block *other_wb;
-
-	if (!find_guid(guid_string, &other_wb))
-		return NULL;
-
-	if (other_wb->acpi_device != this_wb->acpi_device)
-		return NULL;
-
-	get_device(&other_wb->dev.dev);
-	return &other_wb->dev;
-}
-EXPORT_SYMBOL_GPL(wmidev_get_other_guid);
-
 /**
  * wmi_set_block - Write to a WMI block
  * @guid_string: 36 char string of the form fa50ff2b-f2e8-45de-83fa-65417f2f49ba
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 2cd10c3b89e9..ddee427e0721 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -36,10 +36,6 @@ extern acpi_status wmidev_evaluate_method(struct wmi_device *wdev,
 extern union acpi_object *wmidev_block_query(struct wmi_device *wdev,
 					     u8 instance);
 
-/* Gets another device on the same bus.  Caller must put_device the result. */
-extern struct wmi_device *wmidev_get_other_guid(struct wmi_device *wdev,
-						const char *guid_string);
-
 struct wmi_device_id {
 	const char *guid_string;
 };
-- 
2.14.1

[PATCH v7 07/15] platform/x86: dell-smbios: only run if proper oem string is detected

[Mario Limonciello]

The proper way to indicate that a system is a 'supported' Dell System
is by the presence of this string in OEM strings.

Allowing the driver to load on non-Dell systems will have undefined
results.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-smbios.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index e9b1ca07c872..7e779278d054 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -172,8 +172,15 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 
 static int __init dell_smbios_init(void)
 {
+	const struct dmi_device *valid;
 	int ret;
 
+	valid = dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL);
+	if (!valid) {
+		pr_err("Unable to run on non-Dell system\n");
+		return -ENODEV;
+	}
+
 	dmi_walk(find_tokens, NULL);
 
 	if (!da_tokens)  {
-- 
2.14.1

[PATCH v7 08/15] platform/x86: dell-smbios: Add a sysfs interface for SMBIOS tokens

[Mario Limonciello]

Currently userspace tools can access system tokens via the dcdbas
kernel module and a SMI call that will cause the platform to execute
SMM code.

With a goal in mind of deprecating the dcdbas kernel module a different
method for accessing these tokens from userspace needs to be created.

This is intentionally marked to only be readable as root as it can
contain sensitive information about the platform's configuration.

While adding this interface I found that some tokens are duplicated.
These need to be ignored from sysfs to avoid duplicate files.

MAINTAINERS was missing for this driver.  Add myself and Pali to
maintainers list for it.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 .../ABI/testing/sysfs-platform-dell-smbios         |  20 ++
 MAINTAINERS                                        |   7 +
 drivers/platform/x86/dell-smbios.c                 | 205 ++++++++++++++++++++-
 3 files changed, 231 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-smbios

diff --git a/Documentation/ABI/testing/sysfs-platform-dell-smbios b/Documentation/ABI/testing/sysfs-platform-dell-smbios
new file mode 100644
index 000000000000..6700189832f5
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-smbios
@@ -0,0 +1,20 @@
+What:		/sys/devices/platform/<platform>/tokens/*
+Date:		November 2017
+KernelVersion:	4.15
+Contact:	"Mario Limonciello" <mario.limonciello@dell.com>
+Description:
+		A read-only description of Dell platform tokens
+		available on the machine.
+
+		Each token attribute is available as a pair of
+		sysfs attributes readable by root.
+
+		For example the token ID "5" would be available
+		as the following attributes:
+
+		0005_location
+		0005_value
+
+		Tokens will vary from machine to machine, and
+		only tokens available on that machine will be
+		displayed.
diff --git a/MAINTAINERS b/MAINTAINERS
index f4cf35950b08..09e774f1d0b2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3978,6 +3978,13 @@ M:	"Maciej W. Rozycki" <macro@linux-mips.org>
 S:	Maintained
 F:	drivers/net/fddi/defxx.*
 
+DELL SMBIOS DRIVER
+M:	Pali Rohár <pali.rohar@gmail.com>
+M:	Mario Limonciello <mario.limonciello@dell.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/dell-smbios.*
+
 DELL LAPTOP DRIVER
 M:	Matthew Garrett <mjg59@srcf.ucam.org>
 M:	Pali Rohár <pali.rohar@gmail.com>
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 7e779278d054..ade248646578 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -23,6 +23,7 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include "../../firmware/dcdbas.h"
+#include <linux/platform_device.h>
 #include "dell-smbios.h"
 
 struct calling_interface_structure {
@@ -39,7 +40,11 @@ static DEFINE_MUTEX(buffer_mutex);
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
+static struct platform_device *platform_device;
 static struct calling_interface_token *da_tokens;
+static struct device_attribute *token_location_attrs;
+static struct device_attribute *token_value_attrs;
+static struct attribute **token_attrs;
 
 int dell_smbios_error(int value)
 {
@@ -157,6 +162,26 @@ static void __init parse_da_table(const struct dmi_header *dm)
 	da_num_tokens += tokens;
 }
 
+static void zero_duplicates(struct device *dev)
+{
+	int i, j;
+
+	for (i = 0; i < da_num_tokens; i++) {
+		for (j = i+1; j < da_num_tokens; j++) {
+			if (da_tokens[i].tokenID == 0 ||
+			    da_tokens[j].tokenID == 0)
+				continue;
+			if (da_tokens[i].tokenID == da_tokens[j].tokenID) {
+				dev_dbg(dev, "Zeroing dup token ID %x(%x/%x)\n",
+					da_tokens[j].tokenID,
+					da_tokens[j].location,
+					da_tokens[j].value);
+				da_tokens[j].tokenID = 0;
+			}
+		}
+	}
+}
+
 static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 {
 	switch (dm->type) {
@@ -170,6 +195,148 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+static int match_attribute(struct device *dev,
+			   struct device_attribute *attr)
+{
+	int i;
+
+	for (i = 0; i < da_num_tokens * 2; i++) {
+		if (!token_attrs[i])
+			continue;
+		if (strcmp(token_attrs[i]->name, attr->attr.name) == 0)
+			return i/2;
+	}
+	dev_dbg(dev, "couldn't match: %s\n", attr->attr.name);
+	return -EINVAL;
+}
+
+static ssize_t location_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	int i;
+
+	i = match_attribute(dev, attr);
+	if (i > 0)
+		return scnprintf(buf, PAGE_SIZE, "%08x", da_tokens[i].location);
+	return 0;
+}
+
+static ssize_t value_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	int i;
+
+	i = match_attribute(dev, attr);
+	if (i > 0)
+		return scnprintf(buf, PAGE_SIZE, "%08x", da_tokens[i].value);
+	return 0;
+}
+
+static struct attribute_group smbios_attribute_group = {
+	.name = "tokens"
+};
+
+static struct platform_driver platform_driver = {
+	.driver = {
+		.name = "dell-smbios",
+	},
+};
+
+static int build_tokens_sysfs(struct platform_device *dev)
+{
+	char buffer_location[13];
+	char buffer_value[10];
+	char *location_name;
+	char *value_name;
+	size_t size;
+	int ret;
+	int i, j;
+
+	/* (number of tokens  + 1 for null terminated */
+	size = sizeof(struct device_attribute) * (da_num_tokens + 1);
+	token_location_attrs = kzalloc(size, GFP_KERNEL);
+	if (!token_location_attrs)
+		return -ENOMEM;
+	token_value_attrs = kzalloc(size, GFP_KERNEL);
+	if (!token_value_attrs)
+		goto out_allocate_value;
+
+	/* need to store both location and value + terminator*/
+	size = sizeof(struct attribute *) * ((2 * da_num_tokens) + 1);
+	token_attrs = kzalloc(size, GFP_KERNEL);
+	if (!token_attrs)
+		goto out_allocate_attrs;
+
+	for (i = 0, j = 0; i < da_num_tokens; i++) {
+		/* skip empty */
+		if (da_tokens[i].tokenID == 0)
+			continue;
+		/* add location */
+		sprintf(buffer_location, "%04x_location",
+			da_tokens[i].tokenID);
+		location_name = kstrdup(buffer_location, GFP_KERNEL);
+		if (location_name == NULL)
+			goto out_unwind_strings;
+		sysfs_attr_init(&token_location_attrs[i]);
+		token_location_attrs[i].attr.name = location_name;
+		token_location_attrs[i].attr.mode = 0440;
+		token_location_attrs[i].show = location_show;
+		token_attrs[j++] = &token_location_attrs[i].attr;
+
+		/* add value */
+		sprintf(buffer_value, "%04x_value",
+			da_tokens[i].tokenID);
+		value_name = kstrdup(buffer_value, GFP_KERNEL);
+		if (value_name == NULL)
+			goto loop_fail_create_value;
+		sysfs_attr_init(&token_value_attrs[i]);
+		token_value_attrs[i].attr.name = value_name;
+		token_value_attrs[i].attr.mode = 0440;
+		token_value_attrs[i].show = value_show;
+		token_attrs[j++] = &token_value_attrs[i].attr;
+		continue;
+
+loop_fail_create_value:
+		kfree(value_name);
+		goto out_unwind_strings;
+	}
+	smbios_attribute_group.attrs = token_attrs;
+
+	ret = sysfs_create_group(&dev->dev.kobj, &smbios_attribute_group);
+	if (ret)
+		goto out_unwind_strings;
+	return 0;
+
+out_unwind_strings:
+	for (i = i-1; i > 0; i--) {
+		kfree(token_location_attrs[i].attr.name);
+		kfree(token_value_attrs[i].attr.name);
+	}
+	kfree(token_attrs);
+out_allocate_attrs:
+	kfree(token_value_attrs);
+out_allocate_value:
+	kfree(token_location_attrs);
+
+	return -ENOMEM;
+}
+
+static void free_group(struct platform_device *pdev)
+{
+	int i;
+
+	sysfs_remove_group(&pdev->dev.kobj,
+				&smbios_attribute_group);
+	for (i = 0; i < da_num_tokens; i++) {
+		kfree(token_location_attrs[i].attr.name);
+		kfree(token_value_attrs[i].attr.name);
+	}
+	kfree(token_attrs);
+	kfree(token_value_attrs);
+	kfree(token_location_attrs);
+}
+
+
 static int __init dell_smbios_init(void)
 {
 	const struct dmi_device *valid;
@@ -197,9 +364,40 @@ static int __init dell_smbios_init(void)
 		ret = -ENOMEM;
 		goto fail_buffer;
 	}
+	ret = platform_driver_register(&platform_driver);
+	if (ret)
+		goto fail_platform_driver;
+
+	platform_device = platform_device_alloc("dell-smbios", 0);
+	if (!platform_device) {
+		ret = -ENOMEM;
+		goto fail_platform_device_alloc;
+	}
+	ret = platform_device_add(platform_device);
+	if (ret)
+		goto fail_platform_device_add;
+
+	/* duplicate tokens will cause problems building sysfs files */
+	zero_duplicates(&platform_device->dev);
+
+	ret = build_tokens_sysfs(platform_device);
+	if (ret)
+		goto fail_create_group;
 
 	return 0;
 
+fail_create_group:
+	platform_device_del(platform_device);
+
+fail_platform_device_add:
+	platform_device_put(platform_device);
+
+fail_platform_device_alloc:
+	platform_driver_unregister(&platform_driver);
+
+fail_platform_driver:
+	free_page((unsigned long)buffer);
+
 fail_buffer:
 	kfree(da_tokens);
 	return ret;
@@ -207,8 +405,13 @@ static int __init dell_smbios_init(void)
 
 static void __exit dell_smbios_exit(void)
 {
-	kfree(da_tokens);
+	if (platform_device) {
+		free_group(platform_device);
+		platform_device_unregister(platform_device);
+		platform_driver_unregister(&platform_driver);
+	}
 	free_page((unsigned long)buffer);
+	kfree(da_tokens);
 }
 
 subsys_initcall(dell_smbios_init);
-- 
2.14.1

[PATCH v7 09/15] platform/x86: dell-smbios: Introduce dispatcher for SMM calls

[Mario Limonciello]

This splits up the dell-smbios driver into two drivers:
* dell-smbios
* dell-smbios-smm

dell-smbios can operate with multiple different dispatcher drivers to
perform SMBIOS operations.

Also modify the interface that dell-laptop and dell-wmi use align to this
model more closely.  Rather than a single global buffer being allocated
for all drivers, each driver will allocate and be responsible for it's own
buffer. The pointer will be passed to the calling function and each
dispatcher driver will then internally copy it to the proper location to
perform it's call.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 MAINTAINERS                            |   6 +
 drivers/platform/x86/Kconfig           |  15 +-
 drivers/platform/x86/Makefile          |   1 +
 drivers/platform/x86/dell-laptop.c     | 271 +++++++++++++--------------------
 drivers/platform/x86/dell-smbios-smm.c | 161 ++++++++++++++++++++
 drivers/platform/x86/dell-smbios.c     | 120 ++++++++-------
 drivers/platform/x86/dell-smbios.h     |  19 ++-
 drivers/platform/x86/dell-wmi.c        |  11 +-
 8 files changed, 369 insertions(+), 235 deletions(-)
 create mode 100644 drivers/platform/x86/dell-smbios-smm.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 09e774f1d0b2..9dc1ee9603e7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3985,6 +3985,12 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/dell-smbios.*
 
+DELL SMBIOS SMM DRIVER
+M:	Mario Limonciello <mario.limonciello@dell.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/dell-smbios-smm.c
+
 DELL LAPTOP DRIVER
 M:	Matthew Garrett <mjg59@srcf.ucam.org>
 M:	Pali Rohár <pali.rohar@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7722923c968c..53a2de781de6 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -93,12 +93,19 @@ config ASUS_LAPTOP
 
 config DELL_SMBIOS
 	tristate
-	select DCDBAS
+
+config DELL_SMBIOS_SMM
+	tristate "Dell SMBIOS calling interface (SMM implementation)"
+	depends on DCDBAS
+	default DCDBAS
+	select DELL_SMBIOS
 	---help---
-	This module provides common functions for kernel modules using
-	Dell SMBIOS.
+	This provides an implementation for the Dell SMBIOS calling interface
+	communicated over SMI/SMM.
 
-	If you have a Dell laptop, say Y or M here.
+	If you have a Dell computer from <=2017 you should say Y or M here.
+	If you aren't sure and this module doesn't work for your computer
+	it just won't load.
 
 config DELL_LAPTOP
 	tristate "Dell Laptop Extras"
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 8636f5d3424f..e743615241f8 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
 obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
+obj-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
 obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index f42159fd2031..c1a3214b6689 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -85,6 +85,7 @@ static struct platform_driver platform_driver = {
 	}
 };
 
+static struct calling_interface_buffer *buffer;
 static struct platform_device *platform_device;
 static struct backlight_device *dell_backlight_device;
 static struct rfkill *wifi_rfkill;
@@ -283,6 +284,27 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 	{ }
 };
 
+void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
+{
+	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer->input[0] = arg0;
+	buffer->input[1] = arg1;
+	buffer->input[2] = arg2;
+	buffer->input[3] = arg3;
+}
+
+int dell_send_request(u16 class, u16 select)
+{
+	int ret;
+
+	buffer->class = class;
+	buffer->select = select;
+	ret = dell_smbios_call(buffer);
+	if (ret != 0)
+		return ret;
+	return dell_smbios_error(buffer->output[0]);
+}
+
 /*
  * Derived from information in smbios-wireless-ctl:
  *
@@ -405,7 +427,6 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
 
 static int dell_rfkill_set(void *data, bool blocked)
 {
-	struct calling_interface_buffer *buffer;
 	int disable = blocked ? 1 : 0;
 	unsigned long radio = (unsigned long)data;
 	int hwswitch_bit = (unsigned long)data - 1;
@@ -413,20 +434,16 @@ static int dell_rfkill_set(void *data, bool blocked)
 	int status;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
-
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0, 0, 0);
+	ret = dell_send_request(17, 11);
+	if (ret)
+		return ret;
 	status = buffer->output[1];
 
-	if (ret != 0)
-		goto out;
-
-	dell_smbios_clear_buffer();
-
-	buffer->input[0] = 0x2;
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0x2, 0, 0, 0);
+	ret = dell_send_request(17, 11);
+	if (ret)
+		return ret;
 	hwswitch = buffer->output[1];
 
 	/* If the hardware switch controls this radio, and the hardware
@@ -435,28 +452,19 @@ static int dell_rfkill_set(void *data, bool blocked)
 	    (status & BIT(0)) && !(status & BIT(16)))
 		disable = 1;
 
-	dell_smbios_clear_buffer();
-
-	buffer->input[0] = (1 | (radio<<8) | (disable << 16));
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
-
- out:
-	dell_smbios_release_buffer();
-	return dell_smbios_error(ret);
+	dell_set_arguments(1 | (radio<<8) | (disable << 16), 0, 0, 0);
+	ret = dell_send_request(17, 11);
+	return ret;
 }
 
-/* Must be called with the buffer held */
 static void dell_rfkill_update_sw_state(struct rfkill *rfkill, int radio,
-					int status,
-					struct calling_interface_buffer *buffer)
+					int status)
 {
 	if (status & BIT(0)) {
 		/* Has hw-switch, sync sw_state to BIOS */
 		int block = rfkill_blocked(rfkill);
-		dell_smbios_clear_buffer();
-		buffer->input[0] = (1 | (radio << 8) | (block << 16));
-		dell_smbios_send_request(17, 11);
+		dell_set_arguments(1 | (radio << 8) | (block << 16), 0, 0, 0);
+		dell_send_request(17, 11);
 	} else {
 		/* No hw-switch, sync BIOS state to sw_state */
 		rfkill_set_sw_state(rfkill, !!(status & BIT(radio + 16)));
@@ -472,32 +480,23 @@ static void dell_rfkill_update_hw_state(struct rfkill *rfkill, int radio,
 
 static void dell_rfkill_query(struct rfkill *rfkill, void *data)
 {
-	struct calling_interface_buffer *buffer;
 	int radio = ((unsigned long)data & 0xF);
 	int hwswitch;
 	int status;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
-
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0, 0, 0);
+	ret = dell_send_request(17, 11);
 	status = buffer->output[1];
 
 	if (ret != 0 || !(status & BIT(0))) {
-		dell_smbios_release_buffer();
 		return;
 	}
 
-	dell_smbios_clear_buffer();
-
-	buffer->input[0] = 0x2;
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0x2, 0, 0);
+	ret = dell_send_request(17, 11);
 	hwswitch = buffer->output[1];
 
-	dell_smbios_release_buffer();
-
 	if (ret != 0)
 		return;
 
@@ -513,27 +512,23 @@ static struct dentry *dell_laptop_dir;
 
 static int dell_debugfs_show(struct seq_file *s, void *data)
 {
-	struct calling_interface_buffer *buffer;
 	int hwswitch_state;
 	int hwswitch_ret;
 	int status;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
-
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0, 0, 0);
+	ret = dell_send_request(17, 11);
+	if (ret)
+		return ret;
 	status = buffer->output[1];
 
-	dell_smbios_clear_buffer();
-
-	buffer->input[0] = 0x2;
-	dell_smbios_send_request(17, 11);
-	hwswitch_ret = buffer->output[0];
+	dell_set_arguments(0, 0x2, 0, 0);
+	hwswitch_ret = dell_send_request(17, 11);
+	if (hwswitch_ret)
+		return hwswitch_ret;
 	hwswitch_state = buffer->output[1];
 
-	dell_smbios_release_buffer();
-
 	seq_printf(s, "return:\t%d\n", ret);
 	seq_printf(s, "status:\t0x%X\n", status);
 	seq_printf(s, "Bit 0 : Hardware switch supported:   %lu\n",
@@ -613,46 +608,36 @@ static const struct file_operations dell_debugfs_fops = {
 
 static void dell_update_rfkill(struct work_struct *ignored)
 {
-	struct calling_interface_buffer *buffer;
 	int hwswitch = 0;
 	int status;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
-
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0, 0, 0);
+	ret = dell_send_request(17, 11);
 	status = buffer->output[1];
 
 	if (ret != 0)
-		goto out;
-
-	dell_smbios_clear_buffer();
+		return;
 
-	buffer->input[0] = 0x2;
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0x2, 0, 0);
+	ret = dell_send_request(17, 11);
 
 	if (ret == 0 && (status & BIT(0)))
 		hwswitch = buffer->output[1];
 
 	if (wifi_rfkill) {
 		dell_rfkill_update_hw_state(wifi_rfkill, 1, status, hwswitch);
-		dell_rfkill_update_sw_state(wifi_rfkill, 1, status, buffer);
+		dell_rfkill_update_sw_state(wifi_rfkill, 1, status);
 	}
 	if (bluetooth_rfkill) {
 		dell_rfkill_update_hw_state(bluetooth_rfkill, 2, status,
 					    hwswitch);
-		dell_rfkill_update_sw_state(bluetooth_rfkill, 2, status,
-					    buffer);
+		dell_rfkill_update_sw_state(bluetooth_rfkill, 2, status);
 	}
 	if (wwan_rfkill) {
 		dell_rfkill_update_hw_state(wwan_rfkill, 3, status, hwswitch);
-		dell_rfkill_update_sw_state(wwan_rfkill, 3, status, buffer);
+		dell_rfkill_update_sw_state(wwan_rfkill, 3, status);
 	}
-
- out:
-	dell_smbios_release_buffer();
 }
 static DECLARE_DELAYED_WORK(dell_rfkill_work, dell_update_rfkill);
 
@@ -696,7 +681,6 @@ static struct notifier_block dell_laptop_rbtn_notifier = {
 
 static int __init dell_setup_rfkill(void)
 {
-	struct calling_interface_buffer *buffer;
 	int status, ret, whitelisted;
 	const char *product;
 
@@ -712,11 +696,9 @@ static int __init dell_setup_rfkill(void)
 	if (!force_rfkill && !whitelisted)
 		return 0;
 
-	buffer = dell_smbios_get_buffer();
-	dell_smbios_send_request(17, 11);
-	ret = buffer->output[0];
+	dell_set_arguments(0, 0, 0, 0);
+	ret = dell_send_request(17, 11);
 	status = buffer->output[1];
-	dell_smbios_release_buffer();
 
 	/* dell wireless info smbios call is not supported */
 	if (ret != 0)
@@ -869,7 +851,6 @@ static void dell_cleanup_rfkill(void)
 
 static int dell_send_intensity(struct backlight_device *bd)
 {
-	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -877,24 +858,17 @@ static int dell_send_intensity(struct backlight_device *bd)
 	if (!token)
 		return -ENODEV;
 
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = token->location;
-	buffer->input[1] = bd->props.brightness;
-
+	dell_set_arguments(token->location, bd->props.brightness, 0, 0);
 	if (power_supply_is_system_supplied() > 0)
-		dell_smbios_send_request(1, 2);
+		ret = dell_send_request(1, 2);
 	else
-		dell_smbios_send_request(1, 1);
+		ret = dell_send_request(1, 1);
 
-	ret = dell_smbios_error(buffer->output[0]);
-
-	dell_smbios_release_buffer();
 	return ret;
 }
 
 static int dell_get_intensity(struct backlight_device *bd)
 {
-	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -902,20 +876,14 @@ static int dell_get_intensity(struct backlight_device *bd)
 	if (!token)
 		return -ENODEV;
 
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = token->location;
-
+	dell_set_arguments(token->location, 0, 0, 0);
 	if (power_supply_is_system_supplied() > 0)
-		dell_smbios_send_request(0, 2);
+		ret = dell_send_request(0, 2);
 	else
-		dell_smbios_send_request(0, 1);
+		ret = dell_send_request(0, 1);
 
-	if (buffer->output[0])
-		ret = dell_smbios_error(buffer->output[0]);
-	else
+	if (ret == 0)
 		ret = buffer->output[1];
-
-	dell_smbios_release_buffer();
 	return ret;
 }
 
@@ -1179,20 +1147,13 @@ static DEFINE_MUTEX(kbd_led_mutex);
 
 static int kbd_get_info(struct kbd_info *info)
 {
-	struct calling_interface_buffer *buffer;
 	u8 units;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
-
-	buffer->input[0] = 0x0;
-	dell_smbios_send_request(4, 11);
-	ret = buffer->output[0];
-
-	if (ret) {
-		ret = dell_smbios_error(ret);
-		goto out;
-	}
+	dell_set_arguments(0, 0, 0, 0);
+	ret = dell_send_request(4, 11);
+	if (ret)
+		return ret;
 
 	info->modes = buffer->output[1] & 0xFFFF;
 	info->type = (buffer->output[1] >> 24) & 0xFF;
@@ -1209,8 +1170,6 @@ static int kbd_get_info(struct kbd_info *info)
 	if (units & BIT(3))
 		info->days = (buffer->output[3] >> 24) & 0xFF;
 
- out:
-	dell_smbios_release_buffer();
 	return ret;
 }
 
@@ -1269,19 +1228,12 @@ static int kbd_set_level(struct kbd_state *state, u8 level)
 
 static int kbd_get_state(struct kbd_state *state)
 {
-	struct calling_interface_buffer *buffer;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
-
-	buffer->input[0] = 0x1;
-	dell_smbios_send_request(4, 11);
-	ret = buffer->output[0];
-
-	if (ret) {
-		ret = dell_smbios_error(ret);
-		goto out;
-	}
+	dell_set_arguments(0x1, 0, 0, 0);
+	ret = dell_send_request(4, 11);
+	if (ret)
+		return ret;
 
 	state->mode_bit = ffs(buffer->output[1] & 0xFFFF);
 	if (state->mode_bit != 0)
@@ -1296,31 +1248,27 @@ static int kbd_get_state(struct kbd_state *state)
 	state->timeout_value_ac = (buffer->output[2] >> 24) & 0x3F;
 	state->timeout_unit_ac = (buffer->output[2] >> 30) & 0x3;
 
- out:
-	dell_smbios_release_buffer();
 	return ret;
 }
 
 static int kbd_set_state(struct kbd_state *state)
 {
-	struct calling_interface_buffer *buffer;
 	int ret;
+	u32 input1;
+	u32 input2;
+
+	input1 = BIT(state->mode_bit) & 0xFFFF;
+	input1 |= (state->triggers & 0xFF) << 16;
+	input1 |= (state->timeout_value & 0x3F) << 24;
+	input1 |= (state->timeout_unit & 0x3) << 30;
+	input2 = state->als_setting & 0xFF;
+	input2 |= (state->level & 0xFF) << 16;
+	input2 |= (state->timeout_value_ac & 0x3F) << 24;
+	input2 |= (state->timeout_unit_ac & 0x3) << 30;
+	dell_set_arguments(0x2, input1, input2, 0);
+	ret = dell_send_request(4, 11);
 
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = 0x2;
-	buffer->input[1] = BIT(state->mode_bit) & 0xFFFF;
-	buffer->input[1] |= (state->triggers & 0xFF) << 16;
-	buffer->input[1] |= (state->timeout_value & 0x3F) << 24;
-	buffer->input[1] |= (state->timeout_unit & 0x3) << 30;
-	buffer->input[2] = state->als_setting & 0xFF;
-	buffer->input[2] |= (state->level & 0xFF) << 16;
-	buffer->input[2] |= (state->timeout_value_ac & 0x3F) << 24;
-	buffer->input[2] |= (state->timeout_unit_ac & 0x3) << 30;
-	dell_smbios_send_request(4, 11);
-	ret = buffer->output[0];
-	dell_smbios_release_buffer();
-
-	return dell_smbios_error(ret);
+	return ret;
 }
 
 static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
@@ -1345,7 +1293,6 @@ static int kbd_set_state_safe(struct kbd_state *state, struct kbd_state *old)
 
 static int kbd_set_token_bit(u8 bit)
 {
-	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 	int ret;
 
@@ -1356,19 +1303,14 @@ static int kbd_set_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = token->location;
-	buffer->input[1] = token->value;
-	dell_smbios_send_request(1, 0);
-	ret = buffer->output[0];
-	dell_smbios_release_buffer();
+	dell_set_arguments(token->location, token->value, 0, 0);
+	ret = dell_send_request(1, 0);
 
-	return dell_smbios_error(ret);
+	return ret;
 }
 
 static int kbd_get_token_bit(u8 bit)
 {
-	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 	int ret;
 	int val;
@@ -1380,15 +1322,12 @@ static int kbd_get_token_bit(u8 bit)
 	if (!token)
 		return -EINVAL;
 
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = token->location;
-	dell_smbios_send_request(0, 0);
-	ret = buffer->output[0];
+	dell_set_arguments(token->location, 0, 0, 0);
+	ret = dell_send_request(0, 0);
 	val = buffer->output[1];
-	dell_smbios_release_buffer();
 
 	if (ret)
-		return dell_smbios_error(ret);
+		return ret;
 
 	return (val == token->value);
 }
@@ -2102,7 +2041,6 @@ static struct notifier_block dell_laptop_notifier = {
 
 int dell_micmute_led_set(int state)
 {
-	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 
 	if (state == 0)
@@ -2115,11 +2053,8 @@ int dell_micmute_led_set(int state)
 	if (!token)
 		return -ENODEV;
 
-	buffer = dell_smbios_get_buffer();
-	buffer->input[0] = token->location;
-	buffer->input[1] = token->value;
-	dell_smbios_send_request(1, 0);
-	dell_smbios_release_buffer();
+	dell_set_arguments(token->location, token->value, 0, 0);
+	dell_send_request(1, 0);
 
 	return state;
 }
@@ -2127,7 +2062,6 @@ EXPORT_SYMBOL_GPL(dell_micmute_led_set);
 
 static int __init dell_init(void)
 {
-	struct calling_interface_buffer *buffer;
 	struct calling_interface_token *token;
 	int max_intensity = 0;
 	int ret;
@@ -2158,6 +2092,10 @@ static int __init dell_init(void)
 		goto fail_rfkill;
 	}
 
+	buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	if (!buffer)
+		goto fail_buffer;
+
 	if (quirks && quirks->touchpad_led)
 		touchpad_led_init(&platform_device->dev);
 
@@ -2175,12 +2113,10 @@ static int __init dell_init(void)
 
 	token = dell_smbios_find_token(BRIGHTNESS_TOKEN);
 	if (token) {
-		buffer = dell_smbios_get_buffer();
-		buffer->input[0] = token->location;
-		dell_smbios_send_request(0, 2);
-		if (buffer->output[0] == 0)
+		dell_set_arguments(token->location, 0, 0, 0);
+		ret = dell_send_request(0, 2);
+		if (ret)
 			max_intensity = buffer->output[3];
-		dell_smbios_release_buffer();
 	}
 
 	if (max_intensity) {
@@ -2214,6 +2150,8 @@ static int __init dell_init(void)
 fail_get_brightness:
 	backlight_device_unregister(dell_backlight_device);
 fail_backlight:
+	kfree(buffer);
+fail_buffer:
 	dell_cleanup_rfkill();
 fail_rfkill:
 	platform_device_del(platform_device);
@@ -2233,6 +2171,7 @@ static void __exit dell_exit(void)
 		touchpad_led_exit();
 	kbd_led_exit();
 	backlight_device_unregister(dell_backlight_device);
+	kfree(buffer);
 	dell_cleanup_rfkill();
 	if (platform_device) {
 		platform_device_unregister(platform_device);
diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c
new file mode 100644
index 000000000000..b003d70ef7eb
--- /dev/null
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -0,0 +1,161 @@
+/*
+ *  SMI methods for use with dell-smbios
+ *
+ *  Copyright (c) Red Hat <mjg@redhat.com>
+ *  Copyright (c) 2014 Gabriele Mazzotta <gabriele.mzt@gmail.com>
+ *  Copyright (c) 2014 Pali Rohár <pali.rohar@gmail.com>
+ *  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.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/dmi.h>
+#include <linux/gfp.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/platform_device.h>
+#include "../../firmware/dcdbas.h"
+#include "dell-smbios.h"
+
+static int da_command_address;
+static int da_command_code;
+static struct calling_interface_buffer *buffer;
+struct platform_device *platform_device;
+static DEFINE_MUTEX(smm_mutex);
+
+static const struct dmi_system_id dell_device_table[] __initconst = {
+	{
+		.ident = "Dell laptop",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_CHASSIS_TYPE, "9"), /*Laptop*/
+		},
+	},
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
+			DMI_MATCH(DMI_CHASSIS_TYPE, "10"), /*Notebook*/
+		},
+	},
+	{
+		.ident = "Dell Computer Corporation",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Dell Computer Corporation"),
+			DMI_MATCH(DMI_CHASSIS_TYPE, "8"),
+		},
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(dmi, dell_device_table);
+
+static void __init parse_da_table(const struct dmi_header *dm)
+{
+	struct calling_interface_structure *table =
+		container_of(dm, struct calling_interface_structure, header);
+
+	/* 4 bytes of table header, plus 7 bytes of Dell header, plus at least
+	   6 bytes of entry */
+	if (dm->length < 17)
+		return;
+
+	da_command_address = table->cmdIOAddress;
+	da_command_code = table->cmdIOCode;
+}
+
+static void __init find_cmd_address(const struct dmi_header *dm, void *dummy)
+{
+	switch (dm->type) {
+	case 0xda: /* Calling interface */
+		parse_da_table(dm);
+		break;
+	}
+}
+
+int dell_smbios_smm_call(struct calling_interface_buffer *input)
+{
+	struct smi_cmd command;
+	size_t size;
+
+	size = sizeof(struct calling_interface_buffer);
+	command.magic = SMI_CMD_MAGIC;
+	command.command_address = da_command_address;
+	command.command_code = da_command_code;
+	command.ebx = virt_to_phys(buffer);
+	command.ecx = 0x42534931;
+
+	mutex_lock(&smm_mutex);
+	memcpy(buffer, input, size);
+	dcdbas_smi_request(&command);
+	memcpy(input, buffer, size);
+	mutex_unlock(&smm_mutex);
+	return 0;
+}
+
+static int __init dell_smbios_smm_init(void)
+{
+	int ret;
+	/*
+	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
+	 * is passed to SMI handler.
+	 */
+	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+	if (!buffer)
+		return -ENOMEM;
+
+	dmi_walk(find_cmd_address, NULL);
+
+	platform_device = platform_device_alloc("dell-smbios", 1);
+	if (!platform_device) {
+		ret = -ENOMEM;
+		goto fail_platform_device_alloc;
+	}
+
+	ret = platform_device_add(platform_device);
+	if (ret)
+		goto fail_platform_device_add;
+
+	ret = dell_smbios_register_device(&platform_device->dev,
+					  &dell_smbios_smm_call);
+	if (ret)
+		goto fail_register;
+
+	return 0;
+
+fail_register:
+	platform_device_del(platform_device);
+
+fail_platform_device_add:
+	platform_device_put(platform_device);
+
+fail_platform_device_alloc:
+	free_page((unsigned long)buffer);
+	return ret;
+}
+
+static void __exit dell_smbios_smm_exit(void)
+{
+	if (platform_device) {
+		dell_smbios_unregister_device(&platform_device->dev);
+		platform_device_unregister(platform_device);
+		free_page((unsigned long)buffer);
+	}
+}
+
+subsys_initcall(dell_smbios_smm_init);
+module_exit(dell_smbios_smm_exit);
+
+MODULE_AUTHOR("Matthew Garrett <mjg@redhat.com>");
+MODULE_AUTHOR("Gabriele Mazzotta <gabriele.mzt@gmail.com>");
+MODULE_AUTHOR("Pali Rohár <pali.rohar@gmail.com>");
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Dell SMBIOS communications over SMI");
+MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index ade248646578..a2afd7cb24fc 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -18,33 +18,27 @@
 #include <linux/module.h>
 #include <linux/dmi.h>
 #include <linux/err.h>
-#include <linux/gfp.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/io.h>
-#include "../../firmware/dcdbas.h"
 #include <linux/platform_device.h>
 #include "dell-smbios.h"
 
-struct calling_interface_structure {
-	struct dmi_header header;
-	u16 cmdIOAddress;
-	u8 cmdIOCode;
-	u32 supportedCmds;
-	struct calling_interface_token tokens[];
-} __packed;
-
-static struct calling_interface_buffer *buffer;
-static DEFINE_MUTEX(buffer_mutex);
-
-static int da_command_address;
-static int da_command_code;
 static int da_num_tokens;
 static struct platform_device *platform_device;
 static struct calling_interface_token *da_tokens;
 static struct device_attribute *token_location_attrs;
 static struct device_attribute *token_value_attrs;
 static struct attribute **token_attrs;
+static DEFINE_MUTEX(smbios_mutex);
+
+struct smbios_device {
+	struct list_head list;
+	struct device *device;
+	int (*call_fn)(struct calling_interface_buffer *);
+};
+
+static LIST_HEAD(smbios_device_list);
 
 int dell_smbios_error(int value)
 {
@@ -61,42 +55,71 @@ int dell_smbios_error(int value)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_error);
 
-struct calling_interface_buffer *dell_smbios_get_buffer(void)
+int dell_smbios_register_device(struct device *d, void *call_fn)
 {
-	mutex_lock(&buffer_mutex);
-	dell_smbios_clear_buffer();
-	return buffer;
-}
-EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
+	struct smbios_device *priv;
 
-void dell_smbios_clear_buffer(void)
-{
-	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+	priv = devm_kzalloc(d, sizeof(struct smbios_device), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	get_device(d);
+	priv->device = d;
+	priv->call_fn = call_fn;
+	mutex_lock(&smbios_mutex);
+	list_add_tail(&priv->list, &smbios_device_list);
+	mutex_unlock(&smbios_mutex);
+	dev_dbg(d, "Added device: %s\n", d->driver->name);
+	return 0;
 }
-EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
+EXPORT_SYMBOL_GPL(dell_smbios_register_device);
 
-void dell_smbios_release_buffer(void)
+void dell_smbios_unregister_device(struct device *d)
 {
-	mutex_unlock(&buffer_mutex);
+	struct smbios_device *priv;
+
+	mutex_lock(&smbios_mutex);
+	list_for_each_entry(priv, &smbios_device_list, list) {
+		if (priv->device == d) {
+			list_del(&priv->list);
+			put_device(d);
+			break;
+		}
+	};
+	mutex_unlock(&smbios_mutex);
+	dev_dbg(d, "Remove device: %s\n", d->driver->name);
 }
-EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);
+EXPORT_SYMBOL_GPL(dell_smbios_unregister_device);
 
-void dell_smbios_send_request(int class, int select)
+int dell_smbios_call(struct calling_interface_buffer *buffer)
 {
-	struct smi_cmd command;
+	int (*call_fn)(struct calling_interface_buffer *) = NULL;
+	struct device *selected_dev = NULL;
+	struct smbios_device *priv;
+	int ret;
 
-	command.magic = SMI_CMD_MAGIC;
-	command.command_address = da_command_address;
-	command.command_code = da_command_code;
-	command.ebx = virt_to_phys(buffer);
-	command.ecx = 0x42534931;
+	mutex_lock(&smbios_mutex);
+	list_for_each_entry(priv, &smbios_device_list, list) {
+		if (!selected_dev || priv->device->id >= selected_dev->id) {
+			dev_dbg(priv->device, "Trying device ID: %d\n",
+				priv->device->id);
+			call_fn = priv->call_fn;
+			selected_dev = priv->device;
+		}
+	};
 
-	buffer->class = class;
-	buffer->select = select;
+	if (!selected_dev) {
+		ret = -ENODEV;
+		pr_err("No dell-smbios drivers are loaded\n");
+		goto out_smbios_call;
+	}
 
-	dcdbas_smi_request(&command);
+	ret = call_fn(buffer);
+
+out_smbios_call:
+	mutex_unlock(&smbios_mutex);
+	return ret;
 }
-EXPORT_SYMBOL_GPL(dell_smbios_send_request);
+EXPORT_SYMBOL_GPL(dell_smbios_call);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid)
 {
@@ -145,9 +168,6 @@ static void __init parse_da_table(const struct dmi_header *dm)
 	if (dm->length < 17)
 		return;
 
-	da_command_address = table->cmdIOAddress;
-	da_command_code = table->cmdIOCode;
-
 	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
 				 sizeof(struct calling_interface_token),
 				 GFP_KERNEL);
@@ -336,7 +356,6 @@ static void free_group(struct platform_device *pdev)
 	kfree(token_location_attrs);
 }
 
-
 static int __init dell_smbios_init(void)
 {
 	const struct dmi_device *valid;
@@ -355,15 +374,6 @@ static int __init dell_smbios_init(void)
 		return -ENODEV;
 	}
 
-	/*
-	 * Allocate buffer below 4GB for SMI data--only 32-bit physical addr
-	 * is passed to SMI handler.
-	 */
-	buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
-	if (!buffer) {
-		ret = -ENOMEM;
-		goto fail_buffer;
-	}
 	ret = platform_driver_register(&platform_driver);
 	if (ret)
 		goto fail_platform_driver;
@@ -396,22 +406,20 @@ static int __init dell_smbios_init(void)
 	platform_driver_unregister(&platform_driver);
 
 fail_platform_driver:
-	free_page((unsigned long)buffer);
-
-fail_buffer:
 	kfree(da_tokens);
 	return ret;
 }
 
 static void __exit dell_smbios_exit(void)
 {
+	mutex_lock(&smbios_mutex);
 	if (platform_device) {
 		free_group(platform_device);
 		platform_device_unregister(platform_device);
 		platform_driver_unregister(&platform_driver);
 	}
-	free_page((unsigned long)buffer);
 	kfree(da_tokens);
+	mutex_unlock(&smbios_mutex);
 }
 
 subsys_initcall(dell_smbios_init);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 45cbc2292cd3..911916be73d4 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -16,6 +16,8 @@
 #ifndef _DELL_SMBIOS_H_
 #define _DELL_SMBIOS_H_
 
+#include <linux/device.h>
+
 struct notifier_block;
 
 /* This structure will be modified by the firmware when we enter
@@ -37,12 +39,19 @@ struct calling_interface_token {
 	};
 };
 
-int dell_smbios_error(int value);
+struct calling_interface_structure {
+	struct dmi_header header;
+	u16 cmdIOAddress;
+	u8 cmdIOCode;
+	u32 supportedCmds;
+	struct calling_interface_token tokens[];
+} __packed;
 
-struct calling_interface_buffer *dell_smbios_get_buffer(void);
-void dell_smbios_clear_buffer(void);
-void dell_smbios_release_buffer(void);
-void dell_smbios_send_request(int class, int select);
+int dell_smbios_register_device(struct device *d, void *call_fn);
+void dell_smbios_unregister_device(struct device *d);
+
+int dell_smbios_error(int value);
+int dell_smbios_call(struct calling_interface_buffer *buffer);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
 
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index b74d328c9da2..b67cfa3bc90d 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -638,13 +638,16 @@ static int dell_wmi_events_set_enabled(bool enable)
 	struct calling_interface_buffer *buffer;
 	int ret;
 
-	buffer = dell_smbios_get_buffer();
+	buffer = kzalloc(sizeof(struct calling_interface_buffer), GFP_KERNEL);
+	buffer->class = 17;
+	buffer->select = 3;
 	buffer->input[0] = 0x10000;
 	buffer->input[1] = 0x51534554;
 	buffer->input[3] = enable;
-	dell_smbios_send_request(17, 3);
-	ret = buffer->output[0];
-	dell_smbios_release_buffer();
+	ret = dell_smbios_call(buffer);
+	if (ret == 0)
+		ret = buffer->output[0];
+	kfree(buffer);
 
 	return dell_smbios_error(ret);
 }
-- 
2.14.1

[PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario Limonciello]

There are some categories of tokens and SMBIOS calls that it makes
sense to protect userspace from accessing.  These are calls that
may write to one time use fields or activate hardware debugging
capabilities.  They are not intended for general purpose use.

This same functionality may be be later extended to also intercept
calls that may cause kernel functionality to get out of sync if
the same functions are used by other drivers.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-smbios.c | 127 +++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h |   2 +
 2 files changed, 129 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index a2afd7cb24fc..f21ec4e60e5b 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include "dell-smbios.h"
 
+static u32 da_supported_commands;
 static int da_num_tokens;
 static struct platform_device *platform_device;
 static struct calling_interface_token *da_tokens;
@@ -38,6 +39,57 @@ struct smbios_device {
 	int (*call_fn)(struct calling_interface_buffer *);
 };
 
+
+/* calls that should be blacklisted. may contain diagnostics,
+ * debugging information or are write once functions
+ */
+struct smbios_call {
+	int class;
+	int select;
+};
+
+static struct smbios_call call_blacklist[] = {
+	{01, 07}, /* manufacturing use */
+	{06, 05}, /* manufacturing use */
+	{11, 03}, /* write once */
+	{11, 07}, /* write once */
+	{11, 11}, /* write once */
+	{19, -1}, /* diagnostics */
+};
+
+/* tokens corresponding to diagnostics or write once locations */
+struct token_range {
+	u16 exact_value;
+	u16 min;
+	u16 max;
+};
+
+static struct token_range token_blacklist[] = {
+	{0x0000, 0x0058, 0x0059}, /* ME use */
+	{0x0000, 0x00CD, 0x00D0}, /* raid shadow copy */
+	{0x0000, 0x013A, 0x01FF}, /* sata shadow copy */
+	{0x0000, 0x0175, 0x0176}, /* write once */
+	{0x0000, 0x0195, 0x0197}, /* diagnostics */
+	{0x0000, 0x01DC, 0x01DD}, /* manufacturing use */
+	{0x0000, 0x027D, 0x0284}, /* diagnostics */
+	{0x02E3, 0x0000, 0x0000}, /* manufacturing use */
+	{0x02FF, 0x0000, 0x0000}, /* manufacturing use */
+	{0x0000, 0x0300, 0x0302}, /* manufacturing use */
+	{0x0000, 0x0325, 0x0326}, /* manufacturing use */
+	{0x0000, 0x0332, 0x0335}, /* fan control */
+	{0x0350, 0x0000, 0x0000}, /* manufacturing use */
+	{0x0363, 0x0000, 0x0000}, /* manufacturing use */
+	{0x0368, 0x0000, 0x0000}, /* manufacturing use */
+	{0x0000, 0x03F6, 0x03F7}, /* manufacturing use */
+	{0x0000, 0x049E, 0x049F}, /* manufacturing use */
+	{0x0000, 0x04A0, 0x04A3}, /* disagnostics */
+	{0x0000, 0x04E6, 0x04E7}, /* manufacturing use */
+	{0x0000, 0x4000, 0x7FFF}, /* internal BIOS use */
+	{0x0000, 0x9000, 0x9001}, /* internal BIOS use */
+	{0x0000, 0xA000, 0xBFFF}, /* write only */
+	{0x0000, 0xEFF0, 0xEFFF}, /* internal BIOS use */
+};
+
 static LIST_HEAD(smbios_device_list);
 
 int dell_smbios_error(int value)
@@ -90,6 +142,72 @@ void dell_smbios_unregister_device(struct device *d)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_unregister_device);
 
+int dell_smbios_call_filter(struct device *d,
+			    struct calling_interface_buffer *buffer)
+{
+	int i;
+	u16 t = 0;
+
+	/* can't make calls over 30 */
+	if (buffer->class > 30) {
+		dev_dbg(d, "buffer->class too big: %d\n", buffer->class);
+		return -EINVAL;
+	}
+
+	/* supported calls on the particular system */
+	if (!(da_supported_commands & (1 << buffer->class))) {
+		dev_dbg(d, "invalid command, supported commands: 0x%8x\n",
+			da_supported_commands);
+		return -EINVAL;
+	}
+
+	/* match against call blacklist  */
+	for (i = 0; i < ARRAY_SIZE(call_blacklist); i++) {
+		if (buffer->class != call_blacklist[i].class)
+			continue;
+		if (buffer->select != call_blacklist[i].select &&
+		    call_blacklist[i].select != -1)
+			continue;
+		dev_dbg(d, "blacklisted command: %d/%d\n",
+			buffer->class, buffer->select);
+		return -EINVAL;
+	}
+
+	/* if a token call, find token ID */
+	if ((buffer->class == 0 && buffer->select < 3) ||
+	    (buffer->class == 1 && buffer->select < 3)) {
+		for (i = 0; i < da_num_tokens; i++) {
+			/* find the matching token ID */
+			if (da_tokens[i].location != buffer->input[0])
+				continue;
+			t = da_tokens[i].tokenID;
+			break;
+		}
+	/* not a token call */
+	} else
+		return 0;
+
+	/* token call; but token didn't exist */
+	if (!t) {
+		dev_dbg(d, "token at location %u doesn't exist\n",
+			buffer->input[0]);
+		return -EINVAL;
+	}
+	/* match against token blacklist */
+	for (i = 0; i < ARRAY_SIZE(token_blacklist); i++) {
+		if (token_blacklist[i].exact_value &&
+		    t == token_blacklist[i].exact_value)
+			return -EINVAL;
+		if (!token_blacklist[i].min || !token_blacklist[i].max)
+			continue;
+		if (t >= token_blacklist[i].min && t <= token_blacklist[i].max)
+			return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(dell_smbios_call_filter);
+
 int dell_smbios_call(struct calling_interface_buffer *buffer)
 {
 	int (*call_fn)(struct calling_interface_buffer *) = NULL;
@@ -113,6 +231,13 @@ int dell_smbios_call(struct calling_interface_buffer *buffer)
 		goto out_smbios_call;
 	}
 
+	if (dell_smbios_call_filter(selected_dev, buffer)) {
+		ret = -EINVAL;
+		dev_err(selected_dev, "Invalid call %d/%d:%8x\n",
+			buffer->class, buffer->select, buffer->input[0]);
+		goto out_smbios_call;
+	}
+
 	ret = call_fn(buffer);
 
 out_smbios_call:
@@ -168,6 +293,8 @@ static void __init parse_da_table(const struct dmi_header *dm)
 	if (dm->length < 17)
 		return;
 
+	da_supported_commands = table->supportedCmds;
+
 	new_da_tokens = krealloc(da_tokens, (da_num_tokens + tokens) *
 				 sizeof(struct calling_interface_token),
 				 GFP_KERNEL);
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 911916be73d4..98950298fc51 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -51,6 +51,8 @@ int dell_smbios_register_device(struct device *d, void *call_fn);
 void dell_smbios_unregister_device(struct device *d);
 
 int dell_smbios_error(int value);
+int dell_smbios_call_filter(struct device *d,
+			    struct calling_interface_buffer *buffer);
 int dell_smbios_call(struct calling_interface_buffer *buffer);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid);
-- 
2.14.1

[PATCH v7 11/15] platform/x86: dell-smbios-wmi: Add new WMI dispatcher driver

[Mario Limonciello]

The dell-smbios stack only currently uses an SMI interface which grants
direct access to physical memory to the firmware SMM methods via a pointer.

This dispatcher driver adds a WMI-ACPI interface that is detected by WMI
probe and preferred over the SMI interface in dell-smbios.

Changing this to operate over WMI-ACPI will use an ACPI OperationRegion
for a buffer of data storage when SMM calls are performed.

This is a safer approach to use in kernel drivers as the SMM will
only have access to that OperationRegion.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 MAINTAINERS                            |   6 +
 drivers/platform/x86/Kconfig           |  14 ++
 drivers/platform/x86/Makefile          |   1 +
 drivers/platform/x86/dell-smbios-wmi.c | 229 +++++++++++++++++++++++++++++++++
 4 files changed, 250 insertions(+)
 create mode 100644 drivers/platform/x86/dell-smbios-wmi.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 9dc1ee9603e7..8fd2f0d2ddf6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3991,6 +3991,12 @@ L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
 F:	drivers/platform/x86/dell-smbios-smm.c
 
+DELL SMBIOS WMI DRIVER
+M:	Mario Limonciello <mario.limonciello@dell.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/dell-smbios-wmi.c
+
 DELL LAPTOP DRIVER
 M:	Matthew Garrett <mjg59@srcf.ucam.org>
 M:	Pali Rohár <pali.rohar@gmail.com>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 53a2de781de6..83aee9068c64 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -94,6 +94,20 @@ config ASUS_LAPTOP
 config DELL_SMBIOS
 	tristate
 
+config DELL_SMBIOS_WMI
+	tristate "Dell SMBIOS calling interface (WMI implementation)"
+	depends on ACPI_WMI
+	select DELL_WMI_DESCRIPTOR
+	default ACPI_WMI
+	select DELL_SMBIOS
+	---help---
+	This provides an implementation for the Dell SMBIOS calling interface
+	communicated over ACPI-WMI.
+
+	If you have a Dell computer from >2007 you should say Y or M here.
+	If you aren't sure and this module doesn't work for your computer
+	it just won't load.
+
 config DELL_SMBIOS_SMM
 	tristate "Dell SMBIOS calling interface (SMM implementation)"
 	depends on DCDBAS
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index e743615241f8..1c4234861de0 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_MSI_LAPTOP)	+= msi-laptop.o
 obj-$(CONFIG_ACPI_CMPC)		+= classmate-laptop.o
 obj-$(CONFIG_COMPAL_LAPTOP)	+= compal-laptop.o
 obj-$(CONFIG_DELL_SMBIOS)	+= dell-smbios.o
+obj-$(CONFIG_DELL_SMBIOS_WMI)	+= dell-smbios-wmi.o
 obj-$(CONFIG_DELL_SMBIOS_SMM)	+= dell-smbios-smm.o
 obj-$(CONFIG_DELL_LAPTOP)	+= dell-laptop.o
 obj-$(CONFIG_DELL_WMI)		+= dell-wmi.o
diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
new file mode 100644
index 000000000000..7b554ee0e9e6
--- /dev/null
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -0,0 +1,229 @@
+/*
+ *  WMI methods for use with dell-smbios
+ *
+ *  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.
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/dmi.h>
+#include <linux/list.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/uaccess.h>
+#include <linux/wmi.h>
+#include "dell-smbios.h"
+#include "dell-wmi-descriptor.h"
+static DEFINE_MUTEX(call_mutex);
+static DEFINE_MUTEX(list_mutex);
+static int wmi_supported;
+
+struct misc_bios_flags_structure {
+	struct dmi_header header;
+	u16 flags0;
+} __packed;
+#define FLAG_HAS_ACPI_WMI 0x02
+
+#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+
+struct dell_wmi_extensions {
+	__u32 argattrib;
+	__u32 blength;
+	__u8 data[];
+} __packed;
+
+struct dell_wmi_smbios_buffer {
+	struct calling_interface_buffer std;
+	struct dell_wmi_extensions ext;
+} __packed;
+
+struct wmi_smbios_priv {
+	struct dell_wmi_smbios_buffer *buf;
+	struct list_head list;
+	struct wmi_device *wdev;
+	struct device *child;
+	u32 req_buf_size;
+};
+static LIST_HEAD(wmi_list);
+
+static inline struct wmi_smbios_priv *get_first_smbios_priv(void)
+{
+	return list_first_entry_or_null(&wmi_list,
+					struct wmi_smbios_priv,
+					list);
+}
+
+static int run_smbios_call(struct wmi_device *wdev)
+{
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct wmi_smbios_priv *priv;
+	struct acpi_buffer input;
+	union acpi_object *obj;
+	acpi_status status;
+
+	priv = dev_get_drvdata(&wdev->dev);
+	input.length = priv->req_buf_size;
+	input.pointer = &priv->buf->std;
+
+	dev_dbg(&wdev->dev, "evaluating: %x/%x [%x,%x,%x,%x]\n",
+		priv->buf->std.class, priv->buf->std.select,
+		priv->buf->std.input[0], priv->buf->std.input[1],
+		priv->buf->std.input[2], priv->buf->std.input[3]);
+
+	status = wmidev_evaluate_method(wdev, 0, 1, &input, &output);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	obj = (union acpi_object *)output.pointer;
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		dev_dbg(&wdev->dev, "received type: %d\n", obj->type);
+		if (obj->type == ACPI_TYPE_INTEGER)
+			dev_dbg(&wdev->dev, "SMBIOS call failed: %llu\n",
+				obj->integer.value);
+		return -EIO;
+	}
+	memcpy(&priv->buf->std, obj->buffer.pointer, obj->buffer.length);
+
+	return 0;
+}
+
+int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
+{
+	struct wmi_smbios_priv *priv;
+	size_t difference;
+	size_t size;
+	int ret;
+
+	priv = get_first_smbios_priv();
+	if (!priv)
+		return -ENODEV;
+
+	size = sizeof(struct calling_interface_buffer);
+	difference = priv->req_buf_size - sizeof(u64) - size;
+
+	mutex_lock(&call_mutex);
+	memset(&priv->buf->ext, 0, difference);
+	memcpy(&priv->buf->std, buffer, size);
+	ret = run_smbios_call(priv->wdev);
+	memcpy(buffer, &priv->buf->std, size);
+	mutex_unlock(&call_mutex);
+
+	return ret;
+}
+
+static int dell_smbios_wmi_probe(struct wmi_device *wdev)
+{
+	struct wmi_smbios_priv *priv;
+	int count;
+	int ret;
+
+	priv = devm_kzalloc(&wdev->dev, sizeof(struct wmi_smbios_priv),
+			    GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/* WMI buffer size will be either 4k or 32k depending on machine */
+	if (!dell_wmi_get_size(&priv->req_buf_size))
+		return -EPROBE_DEFER;
+
+	count = get_order(priv->req_buf_size);
+	priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
+	if (!priv->buf)
+		return -ENOMEM;
+
+	/* ID is used by dell-smbios to set priority of drivers */
+	wdev->dev.id = 1;
+	ret = dell_smbios_register_device(&wdev->dev, &dell_smbios_wmi_call);
+	if (ret)
+		goto fail_register;
+
+	priv->wdev = wdev;
+	dev_set_drvdata(&wdev->dev, priv);
+	mutex_lock(&list_mutex);
+	list_add_tail(&priv->list, &wmi_list);
+	mutex_unlock(&list_mutex);
+
+	return 0;
+
+fail_register:
+	free_pages((unsigned long)priv->buf, count);
+	return ret;
+}
+
+static int dell_smbios_wmi_remove(struct wmi_device *wdev)
+{
+	struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
+	int count;
+
+	mutex_lock(&call_mutex);
+	mutex_lock(&list_mutex);
+	list_del(&priv->list);
+	mutex_unlock(&list_mutex);
+	dell_smbios_unregister_device(&wdev->dev);
+	count = get_order(priv->req_buf_size);
+	free_pages((unsigned long)priv->buf, count);
+	mutex_unlock(&call_mutex);
+	return 0;
+}
+
+static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
+	{ .guid_string = DELL_WMI_SMBIOS_GUID },
+	{ },
+};
+
+static void __init parse_b1_table(const struct dmi_header *dm)
+{
+	struct misc_bios_flags_structure *flags =
+	container_of(dm, struct misc_bios_flags_structure, header);
+
+	/* 4 bytes header, 8 bytes flags */
+	if (dm->length < 12)
+		return;
+	if (dm->handle != 0xb100)
+		return;
+	if ((flags->flags0 & FLAG_HAS_ACPI_WMI))
+		wmi_supported = 1;
+}
+
+static void __init find_b1(const struct dmi_header *dm, void *dummy)
+{
+	switch (dm->type) {
+	case 0xb1: /* misc bios flags */
+		parse_b1_table(dm);
+		break;
+	}
+}
+
+static struct wmi_driver dell_smbios_wmi_driver = {
+	.driver = {
+		.name = "dell-smbios",
+	},
+	.probe = dell_smbios_wmi_probe,
+	.remove = dell_smbios_wmi_remove,
+	.id_table = dell_smbios_wmi_id_table,
+};
+
+static int __init init_dell_smbios_wmi(void)
+{
+	dmi_walk(find_b1, NULL);
+
+	if (!wmi_supported)
+		return -ENODEV;
+
+	return wmi_driver_register(&dell_smbios_wmi_driver);
+}
+
+static void __exit exit_dell_smbios_wmi(void)
+{
+	wmi_driver_unregister(&dell_smbios_wmi_driver);
+}
+
+module_init(init_dell_smbios_wmi);
+module_exit(exit_dell_smbios_wmi);
+
+MODULE_ALIAS("wmi:" DELL_WMI_SMBIOS_GUID);
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_DESCRIPTION("Dell SMBIOS communications over WMI");
+MODULE_LICENSE("GPL");
-- 
2.14.1

[PATCH v7 12/15] platform/x86: dell-smbios-smm: test for WSMT

[Mario Limonciello]

WSMT is as an attestation to the OS that the platform won't
modify memory outside of pre-defined areas.

If a platform has WSMT enabled in BIOS setup, SMM calls through
dcdbas will fail.  The only way to access platform data in these
instances is through the WMI SMBIOS calling interface.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-smbios-smm.c | 35 ++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/platform/x86/dell-smbios-smm.c b/drivers/platform/x86/dell-smbios-smm.c
index b003d70ef7eb..0dbd28e78803 100644
--- a/drivers/platform/x86/dell-smbios-smm.c
+++ b/drivers/platform/x86/dell-smbios-smm.c
@@ -26,6 +26,9 @@ static struct calling_interface_buffer *buffer;
 struct platform_device *platform_device;
 static DEFINE_MUTEX(smm_mutex);
 
+/* When enabled this token indicates that SMM won't work */
+#define WSMT_EN_TOKEN	0x04EC
+
 static const struct dmi_system_id dell_device_table[] __initconst = {
 	{
 		.ident = "Dell laptop",
@@ -100,6 +103,30 @@ int dell_smbios_smm_call(struct calling_interface_buffer *input)
 	return 0;
 }
 
+static int test_wsmt_enabled(void)
+{
+	struct calling_interface_token *token;
+
+	/* if token doesn't exist, SMM will work */
+	token = dell_smbios_find_token(WSMT_EN_TOKEN);
+	if (!token)
+		return 0;
+
+	/* if token exists, try to access over SMM */
+	buffer->class = 0;
+	buffer->select = 0;
+	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+	buffer->input[0] = token->location;
+	dell_smbios_smm_call(buffer);
+
+	/* if lookup failed, we know WSMT was enabled */
+	if (buffer->output[0] != 0)
+		return 1;
+
+	/* query token status if it didn't fail */
+	return (buffer->output[1] == token->value);
+}
+
 static int __init dell_smbios_smm_init(void)
 {
 	int ret;
@@ -113,6 +140,13 @@ static int __init dell_smbios_smm_init(void)
 
 	dmi_walk(find_cmd_address, NULL);
 
+	ret = test_wsmt_enabled();
+	pr_debug("WSMT enable test: %d\n", ret);
+	if (ret) {
+		ret = -ENODEV;
+		goto fail_wsmt;
+	}
+
 	platform_device = platform_device_alloc("dell-smbios", 1);
 	if (!platform_device) {
 		ret = -ENOMEM;
@@ -136,6 +170,7 @@ static int __init dell_smbios_smm_init(void)
 fail_platform_device_add:
 	platform_device_put(platform_device);
 
+fail_wsmt:
 fail_platform_device_alloc:
 	free_page((unsigned long)buffer);
 	return ret;
-- 
2.14.1

[PATCH v7 13/15] platform/x86: wmi: Add sysfs attribute for required_buffer_size

[Mario Limonciello]

Method type WMI objects need to be able to describe the size of
the interface that they will expect to use.

Export this information to sysfs and allow vendor drivers to
set it.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/wmi.c | 31 +++++++++++++++++++++++++++++++
 include/linux/wmi.h        |  3 +++
 2 files changed, 34 insertions(+)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index bcb41c1c7f52..63d01f98bf4c 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -72,6 +72,7 @@ struct wmi_block {
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
+	u64 req_buf_size;
 
 	bool read_takes_no_args;
 };
@@ -188,6 +189,26 @@ 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, u8 instance, u64 length)
+{
+	struct wmi_block *wblock;
+
+	wblock = container_of(wdev, struct wmi_block, dev);
+	if (!wblock)
+		return -ENODEV;
+	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
@@ -718,8 +739,18 @@ static struct attribute *wmi_data_attrs[] = {
 };
 ATTRIBUTE_GROUPS(wmi_data);
 
+static ssize_t required_buffer_size_show(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct wmi_block *wblock = dev_to_wblock(dev);
+
+	return sprintf(buf, "%lld\n", wblock->req_buf_size);
+}
+static DEVICE_ATTR_RO(required_buffer_size);
+
 static struct attribute *wmi_method_attrs[] = {
 	&dev_attr_object_id.attr,
+	&dev_attr_required_buffer_size.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(wmi_method);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index ddee427e0721..a9a72a4c5ed8 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -36,6 +36,9 @@ 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, u8 instance,
+				    u64 length);
+
 struct wmi_device_id {
 	const char *guid_string;
 };
-- 
2.14.1

[PATCH v7 14/15] platform/x86: wmi: create userspace interface for drivers

[Mario Limonciello]

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 callback in the wmi_driver
the WMI bus driver will create a character device that maps to that
function.

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 <mario.limonciello@dell.com>
---
 MAINTAINERS                |   1 +
 drivers/platform/x86/wmi.c | 115 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/wmi.h        |   6 +++
 include/uapi/linux/wmi.h   |  24 ++++++++++
 4 files changed, 146 insertions(+)
 create mode 100644 include/uapi/linux/wmi.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8fd2f0d2ddf6..84afbf8ef7d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -384,6 +384,7 @@ ACPI WMI DRIVER
 L:	platform-driver-x86@vger.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@parisc-linux.org>
diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 63d01f98bf4c..ab5b0d1915ad 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,6 +72,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;
@@ -796,12 +800,118 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	return 0;
 }
 
+static long match_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
+			int compat)
+{
+	struct wmi_ioctl_buffer __user *input =
+		(struct wmi_ioctl_buffer __user *) arg;
+	struct wmi_driver *wdriver = NULL;
+	struct wmi_block *wblock = NULL;
+	struct wmi_block *next = NULL;
+	const char *driver_name;
+	u64 size;
+	int ret;
+
+	if (_IOC_TYPE(cmd) != WMI_IOC)
+		return -ENOTTY;
+
+	driver_name = filp->f_path.dentry->d_iname;
+
+	list_for_each_entry_safe(wblock, next, &wmi_block_list, list) {
+		wdriver = container_of(wblock->dev.dev.driver,
+					struct wmi_driver, driver);
+		if (!wdriver)
+			continue;
+		if (strcmp(driver_name, wdriver->driver.name) == 0)
+			break;
+	}
+
+	if (!wdriver)
+		return -ENODEV;
+
+	/* make sure we're not calling a higher instance than exists*/
+	if (_IOC_NR(cmd) >= wblock->gblock.instance_count)
+		return -EINVAL;
+
+	/* check that required buffer size was declared by driver */
+	if (!wblock->req_buf_size) {
+		dev_err(&wblock->dev.dev, "Required buffer size not set\n");
+		return -EINVAL;
+	}
+	if (get_user(size, &input->length)) {
+		dev_dbg(&wblock->dev.dev, "Read length from user failed\n");
+		return -EFAULT;
+	}
+	/* if it's too small, abort */
+	if (size < wblock->req_buf_size) {
+		dev_err(&wblock->dev.dev,
+			"Buffer %lld too small, need at least %lld\n",
+			size, wblock->req_buf_size);
+		return -EINVAL;
+	}
+	/* if it's too big, warn, driver will only use what is needed */
+	if (size > wblock->req_buf_size)
+		dev_warn(&wblock->dev.dev,
+			"Buffer %lld is bigger than required %lld\n",
+			size, wblock->req_buf_size);
+
+	if (!try_module_get(wdriver->driver.owner))
+		return -EBUSY;
+	if (compat) {
+		if (wdriver->compat_ioctl)
+			ret = wdriver->compat_ioctl(&wblock->dev, cmd, arg);
+		else
+			ret = -ENODEV;
+	} else
+		ret = wdriver->unlocked_ioctl(&wblock->dev, cmd, arg);
+	module_put(wdriver->driver.owner);
+
+	return ret;
+}
+static long wmi_unlocked_ioctl(struct file *filp, unsigned int cmd,
+			       unsigned long arg)
+{
+	return match_ioctl(filp, cmd, arg, 0);
+}
+
+static long wmi_compat_ioctl(struct file *filp, unsigned int cmd,
+			     unsigned long arg)
+{
+	return match_ioctl(filp, cmd, arg, 1);
+}
+
+static const struct file_operations wmi_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= wmi_unlocked_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = wmi_compat_ioctl,
+#endif
+};
+
 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;
+
+	/* driver wants a character device made */
+	if (wdriver->unlocked_ioctl) {
+		buf = kmalloc(strlen(wdriver->driver.name) + 4, GFP_KERNEL);
+		if (!buf)
+			return -ENOMEM;
+		sprintf(buf, "wmi/%s", 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);
+			return -ENOMEM;
+		}
+	}
 
 	if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");
@@ -822,6 +932,11 @@ static int wmi_dev_remove(struct device *dev)
 		container_of(dev->driver, struct wmi_driver, driver);
 	int ret = 0;
 
+	if (wdriver->unlocked_ioctl) {
+		misc_deregister(&wblock->misc_dev);
+		kfree(wblock->misc_dev.name);
+	}
+
 	if (wdriver->remove)
 		ret = wdriver->remove(dev_to_wdev(dev));
 
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index a9a72a4c5ed8..1f52dc49a896 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -50,6 +50,12 @@ 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 (*unlocked_ioctl)(struct wmi_device *wdev, unsigned int cmd,
+				unsigned long arg);
+#ifdef CONFIG_COMPAT
+	long (*compat_ioctl)(struct wmi_device *wdev, unsigned int cmd,
+				unsigned long arg);
+#endif
 };
 
 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..d93b4f56add4
--- /dev/null
+++ b/include/uapi/linux/wmi.h
@@ -0,0 +1,24 @@
+/*
+ *  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
+
+/* 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

[PATCH v7 15/15] platform/x86: dell-smbios-wmi: introduce userspace interface

[Mario Limonciello]

It's important for the driver to provide a R/W ioctl to ensure that
two competing userspace processes don't race to provide or read each
others data.

This userspace character device will be used to perform SMBIOS calls
from any applications.

It provides an ioctl that will allow passing the WMI calling
interface buffer between userspace and kernel space.

This character device is intended to deprecate the dcdbas kernel module
and the interface that it provides to userspace.

To use the character device the buffer needed for the machine will
also be needed.  This information is exported to a sysfs attribute by
the WMI bus in "required_buffer_size".

The API for interacting with this interface is defined in documentation
as well as the WMI uapi header provides the format of the structures.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 Documentation/ABI/testing/dell-smbios-wmi | 41 ++++++++++++++++
 drivers/platform/x86/dell-smbios-wmi.c    | 81 ++++++++++++++++++++++++++-----
 drivers/platform/x86/dell-smbios.h        | 11 +----
 include/uapi/linux/wmi.h                  | 28 +++++++++++
 4 files changed, 140 insertions(+), 21 deletions(-)
 create mode 100644 Documentation/ABI/testing/dell-smbios-wmi

diff --git a/Documentation/ABI/testing/dell-smbios-wmi b/Documentation/ABI/testing/dell-smbios-wmi
new file mode 100644
index 000000000000..572c3eb53c5c
--- /dev/null
+++ b/Documentation/ABI/testing/dell-smbios-wmi
@@ -0,0 +1,41 @@
+What:		/dev/wmi/dell-smbios
+Date:		November 2017
+KernelVersion:	4.15
+Contact:	"Mario Limonciello" <mario.limonciello@dell.com>
+Description:
+		Perform SMBIOS calls on supported Dell machines.
+		through the Dell ACPI-WMI interface.
+
+		IOCTL's and buffer formats are defined in:
+		<uapi/linux/wmi.h>
+
+		1) To perform a call from userspace, you'll need to first
+		determine the minimum size of the calling interface buffer
+		for your machine.
+		Platforms that contain larger buffers can return larger
+		objects from the system firmware.
+		Commonly this size is either 4k or 32k.
+
+		To determine the size of the buffer, look in the device's
+		directory in sysfs for a "required_buffer_size" attribute.
+
+		2) After you've determined the minimum size of the calling
+		interface buffer, you can allocate a structure that represents
+		the structure documented above.
+
+		3) In the 'length' object store the size of the buffer you
+		determined above and allocated.
+
+		4) In this buffer object, prepare as necessary for the SMBIOS
+		call you're interested in.  Typically SMBIOS buffers have
+		"class", "select", and "input" defined to values that coincide
+		with the data you are interested in.
+		Documenting class/select/input values is outside of the scope
+		of this documentation. Check with the libsmbios project for
+		further documentation on these values.
+
+		6) Run the call by using ioctl() as described in the header.
+
+		7) The output will be returned in the buffer object.
+
+		8) Be sure to free up your allocated object.
diff --git a/drivers/platform/x86/dell-smbios-wmi.c b/drivers/platform/x86/dell-smbios-wmi.c
index 7b554ee0e9e6..c41e679da4a1 100644
--- a/drivers/platform/x86/dell-smbios-wmi.c
+++ b/drivers/platform/x86/dell-smbios-wmi.c
@@ -29,17 +29,6 @@ struct misc_bios_flags_structure {
 
 #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
 
-struct dell_wmi_extensions {
-	__u32 argattrib;
-	__u32 blength;
-	__u8 data[];
-} __packed;
-
-struct dell_wmi_smbios_buffer {
-	struct calling_interface_buffer std;
-	struct dell_wmi_extensions ext;
-} __packed;
-
 struct wmi_smbios_priv {
 	struct dell_wmi_smbios_buffer *buf;
 	struct list_head list;
@@ -113,6 +102,66 @@ int dell_smbios_wmi_call(struct calling_interface_buffer *buffer)
 	return ret;
 }
 
+static void _debug_ioctl(struct device *d, unsigned int expected,
+			unsigned int cmd)
+{
+	if (_IOC_DIR(expected) != _IOC_DIR(cmd))
+		dev_dbg(d, "Invalid _IOC_DIR: %d\n", _IOC_DIR(cmd));
+	if (_IOC_TYPE(expected) != _IOC_TYPE(cmd))
+		dev_dbg(d, "Invalid _IOC_TYPE: %d\n", _IOC_TYPE(cmd));
+	if (_IOC_NR(expected) != _IOC_NR(cmd))
+		dev_dbg(d, "Invalid _IOC_NR: %d\n", _IOC_NR(cmd));
+	if (_IOC_SIZE(expected) != _IOC_SIZE(cmd))
+		dev_dbg(d, "Invalid _IOC_SIZE: %d\n", _IOC_SIZE(cmd));
+}
+
+static long dell_smbios_wmi_ioctl(struct wmi_device *wdev, unsigned int cmd,
+				  unsigned long arg)
+{
+	void __user *input = (void __user *) arg;
+	struct wmi_smbios_priv *priv;
+	int ret = 0;
+
+	switch (cmd) {
+	case DELL_WMI_SMBIOS_CMD:
+		priv = dev_get_drvdata(&wdev->dev);
+		if (!priv)
+			return -ENODEV;
+		mutex_lock(&call_mutex);
+		/* read the structure from userspace */
+		if (copy_from_user(priv->buf, input, priv->req_buf_size)) {
+			dev_dbg(&wdev->dev, "Copy %d from user failed\n",
+				priv->req_buf_size);
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		/* check for any calls we should avoid */
+		if (dell_smbios_call_filter(&wdev->dev, &priv->buf->std)) {
+			dev_err(&wdev->dev, "Invalid call %d/%d:%8x\n",
+				priv->buf->std.class, priv->buf->std.select,
+				priv->buf->std.input[0]);
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		ret = run_smbios_call(priv->wdev);
+		if (ret != 0)
+			goto fail_smbios_cmd;
+		/* return the result (only up to our internal buffer size) */
+		if (copy_to_user(input, priv->buf, priv->req_buf_size)) {
+			dev_dbg(&wdev->dev, "Copy %d to user failed\n",
+			priv->req_buf_size);
+			ret = -EFAULT;
+		}
+fail_smbios_cmd:
+		mutex_unlock(&call_mutex);
+		break;
+	default:
+		_debug_ioctl(&wdev->dev, DELL_WMI_SMBIOS_CMD, cmd);
+		ret = -ENOIOCTLCMD;
+	}
+	return ret;
+}
+
 static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 {
 	struct wmi_smbios_priv *priv;
@@ -127,6 +176,12 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 	/* WMI buffer size will be either 4k or 32k depending on machine */
 	if (!dell_wmi_get_size(&priv->req_buf_size))
 		return -EPROBE_DEFER;
+	/* add in the length object we will use internally with ioctl */
+	priv->req_buf_size += sizeof(u64);
+
+	ret = set_required_buffer_size(wdev, 0, priv->req_buf_size);
+	if (ret)
+		return ret;
 
 	count = get_order(priv->req_buf_size);
 	priv->buf = (void *)__get_free_pages(GFP_KERNEL, count);
@@ -203,6 +258,10 @@ static struct wmi_driver dell_smbios_wmi_driver = {
 	.probe = dell_smbios_wmi_probe,
 	.remove = dell_smbios_wmi_remove,
 	.id_table = dell_smbios_wmi_id_table,
+	.unlocked_ioctl = dell_smbios_wmi_ioctl,
+#ifdef CONFIG_COMPAT
+	.compat_ioctl = dell_smbios_wmi_ioctl,
+#endif
 };
 
 static int __init init_dell_smbios_wmi(void)
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 98950298fc51..41918ff8cdde 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -17,19 +17,10 @@
 #define _DELL_SMBIOS_H_
 
 #include <linux/device.h>
+#include <uapi/linux/wmi.h>
 
 struct notifier_block;
 
-/* This structure will be modified by the firmware when we enter
- * system management mode, hence the volatiles */
-
-struct calling_interface_buffer {
-	u16 class;
-	u16 select;
-	volatile u32 input[4];
-	volatile u32 output[4];
-} __packed;
-
 struct calling_interface_token {
 	u16 tokenID;
 	u16 location;
diff --git a/include/uapi/linux/wmi.h b/include/uapi/linux/wmi.h
index d93b4f56add4..622e4004bcfc 100644
--- a/include/uapi/linux/wmi.h
+++ b/include/uapi/linux/wmi.h
@@ -10,6 +10,9 @@
 #ifndef _UAPI_LINUX_WMI_H
 #define _UAPI_LINUX_WMI_H
 
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
 /* WMI bus will filter all WMI vendor driver requests through this IOC */
 #define WMI_IOC 'W'
 
@@ -21,4 +24,29 @@ struct wmi_ioctl_buffer {
 	__u8	data[];
 };
 
+/* This structure may be modified by the firmware when we enter
+ * system management mode through SMM, hence the volatiles
+ */
+struct calling_interface_buffer {
+	__u16 class;
+	__u16 select;
+	volatile __u32 input[4];
+	volatile __u32 output[4];
+} __packed;
+
+struct dell_wmi_extensions {
+	__u32 argattrib;
+	__u32 blength;
+	__u8 data[];
+} __packed;
+
+struct dell_wmi_smbios_buffer {
+	__u64 length;
+	struct calling_interface_buffer std;
+	struct dell_wmi_extensions	ext;
+} __packed;
+
+/* Dell SMBIOS calling IOCTL command used by dell-smbios-wmi */
+#define DELL_WMI_SMBIOS_CMD	_IOWR(WMI_IOC, 0, struct dell_wmi_smbios_buffer)
+
 #endif
-- 
2.14.1

Re: [PATCH v7 04/15] platform/x86: dell-wmi: allow 32k return size in the descriptor

[Pali Rohár]

On Wednesday 11 October 2017 11:27:30 Mario Limonciello wrote:
> Some platforms this year will be adopting 32k WMI buffer, so don't
> complain when encountering those.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  drivers/platform/x86/dell-wmi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> index ece2fe341f01..c8c7f4f9326c 100644
> --- a/drivers/platform/x86/dell-wmi.c
> +++ b/drivers/platform/x86/dell-wmi.c
> @@ -624,7 +624,7 @@ static void dell_wmi_input_destroy(struct wmi_device *wdev)
>   * Vendor Signature          0       4    "DELL"
>   * Object Signature          4       4    " WMI"
>   * WMI Interface Version     8       4    <version>
> - * WMI buffer length        12       4    4096
> + * WMI buffer length        12       4    4096 or 32768
>   */
>  static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
>  {
> @@ -674,7 +674,7 @@ static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
>  		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
>  			buffer[2]);
>  
> -	if (buffer[3] != 4096)
> +	if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)

Looks like that desc_buffer is not declared.

>  		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%u)\n",
>  			buffer[3]);
>  

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH v7 04/15] platform/x86: dell-wmi: allow 32k return size in the descriptor

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, October 11, 2017 11:32 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net;
> mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>
> Subject: Re: [PATCH v7 04/15] platform/x86: dell-wmi: allow 32k return size in the
> descriptor
> 
> On Wednesday 11 October 2017 11:27:30 Mario Limonciello wrote:
> > Some platforms this year will be adopting 32k WMI buffer, so don't
> > complain when encountering those.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  drivers/platform/x86/dell-wmi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index ece2fe341f01..c8c7f4f9326c 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -624,7 +624,7 @@ static void dell_wmi_input_destroy(struct wmi_device
> *wdev)
> >   * Vendor Signature          0       4    "DELL"
> >   * Object Signature          4       4    " WMI"
> >   * WMI Interface Version     8       4    <version>
> > - * WMI buffer length        12       4    4096
> > + * WMI buffer length        12       4    4096 or 32768
> >   */
> >  static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
> >  {
> > @@ -674,7 +674,7 @@ static int dell_wmi_check_descriptor_buffer(struct
> wmi_device *wdev)
> >  		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown
> version (%u)\n",
> >  			buffer[2]);
> >
> > -	if (buffer[3] != 4096)
> > +	if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)
> 
> Looks like that desc_buffer is not declared.

Yep thanks for catching that was a mistake from all the rebasing and not compile testing this
particular commit. ☹

RE: [PATCH v7 04/15] platform/x86: dell-wmi: allow 32k return size in the descriptor

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Wednesday, October 11, 2017 11:32 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com; rjw@rjwysocki.net;
> mjg59@google.com; hch@lst.de; Greg KH <greg@kroah.com>
> Subject: Re: [PATCH v7 04/15] platform/x86: dell-wmi: allow 32k return size in the
> descriptor
> 
> On Wednesday 11 October 2017 11:27:30 Mario Limonciello wrote:
> > Some platforms this year will be adopting 32k WMI buffer, so don't
> > complain when encountering those.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  drivers/platform/x86/dell-wmi.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
> > index ece2fe341f01..c8c7f4f9326c 100644
> > --- a/drivers/platform/x86/dell-wmi.c
> > +++ b/drivers/platform/x86/dell-wmi.c
> > @@ -624,7 +624,7 @@ static void dell_wmi_input_destroy(struct wmi_device
> *wdev)
> >   * Vendor Signature          0       4    "DELL"
> >   * Object Signature          4       4    " WMI"
> >   * WMI Interface Version     8       4    <version>
> > - * WMI buffer length        12       4    4096
> > + * WMI buffer length        12       4    4096 or 32768
> >   */
> >  static int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev)
> >  {
> > @@ -674,7 +674,7 @@ static int dell_wmi_check_descriptor_buffer(struct
> wmi_device *wdev)
> >  		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown
> version (%u)\n",
> >  			buffer[2]);
> >
> > -	if (buffer[3] != 4096)
> > +	if (desc_buffer[3] != 4096 && desc_buffer[3] != 32768)
> 
> Looks like that desc_buffer is not declared.

Yep thanks for catching that was a mistake from all the rebasing and not compile testing this
particular commit. ☹

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

On Wed, 11 Oct 2017 11:27:36 -0500
Mario Limonciello <mario.limonciello@dell.com> wrote:

> There are some categories of tokens and SMBIOS calls that it makes
> sense to protect userspace from accessing.  These are calls that
> may write to one time use fields or activate hardware debugging
> capabilities.  They are not intended for general purpose use.
> 
> This same functionality may be be later extended to also intercept
> calls that may cause kernel functionality to get out of sync if
> the same functions are used by other drivers.

This doesn't work. You are creating an API. If you then have to remove
parts of the API because it messes stuff up you break your API guarantee.

As I said before, this needs to be a whitelist of stuff that is safe, and
it needs to have a security model. When you make a feature available you
can't nicely take it away again as you'll break people's user space.

Start with a whitelist of the ones you know people want to use, or that
your existing tooling you want to enable uses. Add others as needed in
future releases of the kernel.

Alan

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk]
> Sent: Thursday, October 12, 2017 5:09 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; Greg
> KH <greg@kroah.com>
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Wed, 11 Oct 2017 11:27:36 -0500
> Mario Limonciello <mario.limonciello@dell.com> wrote:
> 
> > There are some categories of tokens and SMBIOS calls that it makes
> > sense to protect userspace from accessing.  These are calls that
> > may write to one time use fields or activate hardware debugging
> > capabilities.  They are not intended for general purpose use.
> >
> > This same functionality may be be later extended to also intercept
> > calls that may cause kernel functionality to get out of sync if
> > the same functions are used by other drivers.
> 
> This doesn't work. You are creating an API. If you then have to remove
> parts of the API because it messes stuff up you break your API guarantee.

This potential "problem" already exists in that dcdbas provides a userspace
interface that can manipulate this same data as used by some kernel drivers.
The kernel drivers can be brought out of sync then.

We discussed this a while back, you may not be aware of the discussion.

The jist of it came down to that if a driver in the kernel decides to implement
the same functionality that is available through the calling interface but
with a different interface that the call could be filtered through the calling
interface.

Intercepting/filtering a call doesn't mean breaking API.
It can be handled two ways that I can see and both are valid:

1) Return an error code.  The calling interface already API allows for error 
return codes.  Plenty of calls _already_ return errors.  Returning
one for a filtered call is no different.  This would  be an improvement over how
dcdbas and existing kernel modules handle it.

2) Inspect the buffer and see that it's supposed to be handled by one of
the existing kernel modules.  Pass the call on to the (kernel) driver that already 
handles it, and return the result to userspace.  This will keep the kernel
driver and the userspace return values in sync.

> 
> As I said before, this needs to be a whitelist of stuff that is safe, and
> it needs to have a security model. 

>From the BIOS perspective if no BIOS administrator password is set the
OS and any tools can access any item in the calling interface.  If a BIOS 
administrator password has been configured, many calls will return
error codes unless the application has done a security key exchange with
BIOS.  This is both how dcdbas works today and how this interface that I 
provided to replace it works.

Within Linux the security model is that items accessible through this interface
are only accessible by root.  If userspace tools choose to make items in this 
interface more widely accessible to say authenticated console users that's 
their prerogative.

> When you make a feature available you
> can't nicely take it away again as you'll break people's user space.

As I said above -ENODEV or -EINVAL is not the same thing as taking a feature
away.  The character device API doesn't change when something is filtered.
It's an error scenario.

> 
> Start with a whitelist of the ones you know people want to use, or that
> your existing tooling you want to enable uses. Add others as needed in
> future releases of the kernel.
> 
> Alan

The existing dcdbas calling interface tooling (libsmbios) expects to be able 
to access all calls and all tokens.  *The kernel doesn't filter any of it.*

I understand the ask to filter some calls and that's why patch 10/15 exists,
but please let me remind you this patch series is intended to /replace and
deprecate/ dcdbas userspace access.

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk]
> Sent: Thursday, October 12, 2017 5:09 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; Andy Shevchenko <andy.shevchenko@gmail.com>;
> LKML <linux-kernel@vger.kernel.org>; platform-driver-x86@vger.kernel.org;
> Andy Lutomirski <luto@kernel.org>; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; Greg
> KH <greg@kroah.com>
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Wed, 11 Oct 2017 11:27:36 -0500
> Mario Limonciello <mario.limonciello@dell.com> wrote:
> 
> > There are some categories of tokens and SMBIOS calls that it makes
> > sense to protect userspace from accessing.  These are calls that
> > may write to one time use fields or activate hardware debugging
> > capabilities.  They are not intended for general purpose use.
> >
> > This same functionality may be be later extended to also intercept
> > calls that may cause kernel functionality to get out of sync if
> > the same functions are used by other drivers.
> 
> This doesn't work. You are creating an API. If you then have to remove
> parts of the API because it messes stuff up you break your API guarantee.

This potential "problem" already exists in that dcdbas provides a userspace
interface that can manipulate this same data as used by some kernel drivers.
The kernel drivers can be brought out of sync then.

We discussed this a while back, you may not be aware of the discussion.

The jist of it came down to that if a driver in the kernel decides to implement
the same functionality that is available through the calling interface but
with a different interface that the call could be filtered through the calling
interface.

Intercepting/filtering a call doesn't mean breaking API.
It can be handled two ways that I can see and both are valid:

1) Return an error code.  The calling interface already API allows for error 
return codes.  Plenty of calls _already_ return errors.  Returning
one for a filtered call is no different.  This would  be an improvement over how
dcdbas and existing kernel modules handle it.

2) Inspect the buffer and see that it's supposed to be handled by one of
the existing kernel modules.  Pass the call on to the (kernel) driver that already 
handles it, and return the result to userspace.  This will keep the kernel
driver and the userspace return values in sync.

> 
> As I said before, this needs to be a whitelist of stuff that is safe, and
> it needs to have a security model. 

From the BIOS perspective if no BIOS administrator password is set the
OS and any tools can access any item in the calling interface.  If a BIOS 
administrator password has been configured, many calls will return
error codes unless the application has done a security key exchange with
BIOS.  This is both how dcdbas works today and how this interface that I 
provided to replace it works.

Within Linux the security model is that items accessible through this interface
are only accessible by root.  If userspace tools choose to make items in this 
interface more widely accessible to say authenticated console users that's 
their prerogative.

> When you make a feature available you
> can't nicely take it away again as you'll break people's user space.

As I said above -ENODEV or -EINVAL is not the same thing as taking a feature
away.  The character device API doesn't change when something is filtered.
It's an error scenario.

> 
> Start with a whitelist of the ones you know people want to use, or that
> your existing tooling you want to enable uses. Add others as needed in
> future releases of the kernel.
> 
> Alan

The existing dcdbas calling interface tooling (libsmbios) expects to be able 
to access all calls and all tokens.  *The kernel doesn't filter any of it.*

I understand the ask to filter some calls and that's why patch 10/15 exists,
but please let me remind you this patch series is intended to /replace and
deprecate/ dcdbas userspace access.

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Pali Rohár]

On Thursday 12 October 2017 13:23:08 Mario.Limonciello@dell.com wrote:
> The existing dcdbas calling interface tooling (libsmbios) expects to be able 
> to access all calls and all tokens.  *The kernel doesn't filter any of it.*

It does not mean that API/ABI was designed correctly or incorrectly.
Existing old API/ABI is there and we are not going to change it...

> I understand the ask to filter some calls and that's why patch 10/15 exists,
> but please let me remind you this patch series is intended to /replace and
> deprecate/ dcdbas userspace access.

Now when there is a proposal for a new API/ABI, it should be designed
correctly without need to redesign it again in future and address all
problems which are found during review.

-- 
Pali Rohár
pali.rohar@gmail.com

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, October 12, 2017 9:34 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gnomes@lxorguk.ukuu.org.uk; dvhart@infradead.org;
> andy.shevchenko@gmail.com; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; luto@kernel.org; quasisec@google.com;
> rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; greg@kroah.com
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Thursday 12 October 2017 13:23:08 Mario.Limonciello@dell.com wrote:
> > The existing dcdbas calling interface tooling (libsmbios) expects to be able
> > to access all calls and all tokens.  *The kernel doesn't filter any of it.*
> 
> It does not mean that API/ABI was designed correctly or incorrectly.
> Existing old API/ABI is there and we are not going to change it...
> 
> > I understand the ask to filter some calls and that's why patch 10/15 exists,
> > but please let me remind you this patch series is intended to /replace and
> > deprecate/ dcdbas userspace access.
> 
> Now when there is a proposal for a new API/ABI, it should be designed
> correctly without need to redesign it again in future and address all
> problems which are found during review.
> 

Well sure I also would hate to have to redesign this again in the future.
I believe that this is sufficient now.

I'm looking up commands that FW claims can be supported and filtering the
rest.  There's your whitelist.

I addressed the concern on the perceived dangerous calls (write once, debugging,
manufacturing use only etc) and those are now filtered.  There's your blacklist.

Other than the minor errors that kbuild test robot caught from v6 and the 
s/desc_buffer/buffer/ in an earlier patch, what's left?

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, October 12, 2017 9:34 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: gnomes@lxorguk.ukuu.org.uk; dvhart@infradead.org;
> andy.shevchenko@gmail.com; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; luto@kernel.org; quasisec@google.com;
> rjw@rjwysocki.net; mjg59@google.com; hch@lst.de; greg@kroah.com
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Thursday 12 October 2017 13:23:08 Mario.Limonciello@dell.com wrote:
> > The existing dcdbas calling interface tooling (libsmbios) expects to be able
> > to access all calls and all tokens.  *The kernel doesn't filter any of it.*
> 
> It does not mean that API/ABI was designed correctly or incorrectly.
> Existing old API/ABI is there and we are not going to change it...
> 
> > I understand the ask to filter some calls and that's why patch 10/15 exists,
> > but please let me remind you this patch series is intended to /replace and
> > deprecate/ dcdbas userspace access.
> 
> Now when there is a proposal for a new API/ABI, it should be designed
> correctly without need to redesign it again in future and address all
> problems which are found during review.
> 

Well sure I also would hate to have to redesign this again in the future.
I believe that this is sufficient now.

I'm looking up commands that FW claims can be supported and filtering the
rest.  There's your whitelist.

I addressed the concern on the perceived dangerous calls (write once, debugging,
manufacturing use only etc) and those are now filtered.  There's your blacklist.

Other than the minor errors that kbuild test robot caught from v6 and the 
s/desc_buffer/buffer/ in an earlier patch, what's left?

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Darren Hart]

On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote:
> On Wed, 11 Oct 2017 11:27:36 -0500
> Mario Limonciello <mario.limonciello@dell.com> wrote:
> 
> > There are some categories of tokens and SMBIOS calls that it makes
> > sense to protect userspace from accessing.  These are calls that
> > may write to one time use fields or activate hardware debugging
> > capabilities.  They are not intended for general purpose use.
> > 
> > This same functionality may be be later extended to also intercept
> > calls that may cause kernel functionality to get out of sync if
> > the same functions are used by other drivers.
> 
> This doesn't work. You are creating an API. If you then have to remove
> parts of the API because it messes stuff up you break your API guarantee.
> 
> As I said before, this needs to be a whitelist of stuff that is safe, and
> it needs to have a security model. When you make a feature available you
> can't nicely take it away again as you'll break people's user space.
> 
> Start with a whitelist of the ones you know people want to use, or that
> your existing tooling you want to enable uses. Add others as needed in
> future releases of the kernel.

Hi Alan,

I've attempted to get a focused discussion on this topic a few times,
but have failed to get it the focus in really needs. Thanks for bringing
it up. I look forward to your thoughts on the following:

The core of the issue is we are trying to achieve feature parity with
Windows using the Windows Management Instrumentation (WMI) mechanism.
I'm trying to support vendors like Dell in their attempts to do so, and
acknowledging that this involves supporting a platform which was
designed according to the norms and standards of the windows ecosystem.

This WMI mechanism was designed to enable vendors to create management
tools which could talk to interfaces their firmware exposed for this
purpose without any platform specific OS driver or vendor specific
knowledge on the part of the OS driver [1].

The result of this design decision is as you would expect: there is no
consistency in design, no guarantee of interfaces with functional
boundaries, across the various WMI interface implementations across
vendors (or even within the interface of a given vendor on a given
platform).

The result is a mechanism which is fundamentally incompatible with
standard Linux kernel approach to isolating userspace from the firmware.
WMI enumerates available data, methods, and events, and shepherds a
buffer of bits back and forth between firmware and userspace through
these methods.

Over the last few months, I've worked with Mario, Pali, Andy L, and
others to try and find an acceptable way to support Dell in their
efforts, while respecting Linux's design principles.

The first concession was to agree not to automatically publish a WMI
interface to userspace. This series includes the mechanism which
requires a GUID-specific driver to be written and to provide the file
ops structure, explicitly requesting the character device. This creates
a WMI GUID granular whitelist. I'm willing to deny any such driver which
is sent for review without the explicit collaboration of the vendor, to
ensure they are doing their due diligence with respect to their firmware
implementation. There has been some discussion in this thread regarding
Dell's firmware testing, and the access these interfaces have to
critical hardware resources.

The second concession was to acknowledge that some features implemented
in WMI are rightly the domain of the Linux kernel. LEDs, RFKill,
Backlight control, hotkey support, etc. When an exported GUID is used
for these purposes in addition to whatever purpose userspace requires
(see my comment above about a lack of functional boundaries in the
exported interfaces), we will provide a way to filter these usages. As
Mario said, we can either return an error, or we can attempt to handle
the request via the appropriate Linux kernel subsystem connections.

>From the Linux kernel side, the ask is that we acknowledge that it is
not practical to audit every WMI interface to present a set of
functional knobs to userspace - because they were specifically designed
without that requirement.

The platform was designed in this way specifically to optimize the
software support effort on the part of the vendor. They don't write
Windows OS drivers for these mechanisms, it is unrealistic to expect
them to write them for Linux.

I believe we need to arrive at a compromise which allows a vendor to
work upstream like Mario is doing on behalf of Dell to fully enable
their platforms as they were designed on Linux, while ensuring we don't
adversely affect the functionality or security of other platforms. I
believe the whitelist and blacklist above provides for this compromise.

Perhaps an additional concession we should consider is to make exporting
WMI interfaces a configuration option. If not set, the drivers will
continue to work, but the WMI bus driver will not create the character
device, even if requested. This would allow vendors to create a fully
supported platform using upstream code, while allowing individuals the
control to disable WMI userspace functionality if they don't trust the
mechanism.

The alternative seems to be that we accept these features will not be
available on Linux for these platforms, or that vendors will develop a
single wmi mapping driver out of tree, likely without the noted
concessions.

1. https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
   Specifically: "ACPI-to-WMI Mapper Goals for Windows Instrumentation"

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Darren Hart]

On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote:
> On Wed, 11 Oct 2017 11:27:36 -0500
> Mario Limonciello <mario.limonciello@dell.com> wrote:
> 
> > There are some categories of tokens and SMBIOS calls that it makes
> > sense to protect userspace from accessing.  These are calls that
> > may write to one time use fields or activate hardware debugging
> > capabilities.  They are not intended for general purpose use.
> > 
> > This same functionality may be be later extended to also intercept
> > calls that may cause kernel functionality to get out of sync if
> > the same functions are used by other drivers.
> 
> This doesn't work. You are creating an API. If you then have to remove
> parts of the API because it messes stuff up you break your API guarantee.
> 
> As I said before, this needs to be a whitelist of stuff that is safe, and
> it needs to have a security model. When you make a feature available you
> can't nicely take it away again as you'll break people's user space.
> 
> Start with a whitelist of the ones you know people want to use, or that
> your existing tooling you want to enable uses. Add others as needed in
> future releases of the kernel.

Hi Alan,

I've attempted to get a focused discussion on this topic a few times,
but have failed to get it the focus in really needs. Thanks for bringing
it up. I look forward to your thoughts on the following:

The core of the issue is we are trying to achieve feature parity with
Windows using the Windows Management Instrumentation (WMI) mechanism.
I'm trying to support vendors like Dell in their attempts to do so, and
acknowledging that this involves supporting a platform which was
designed according to the norms and standards of the windows ecosystem.

This WMI mechanism was designed to enable vendors to create management
tools which could talk to interfaces their firmware exposed for this
purpose without any platform specific OS driver or vendor specific
knowledge on the part of the OS driver [1].

The result of this design decision is as you would expect: there is no
consistency in design, no guarantee of interfaces with functional
boundaries, across the various WMI interface implementations across
vendors (or even within the interface of a given vendor on a given
platform).

The result is a mechanism which is fundamentally incompatible with
standard Linux kernel approach to isolating userspace from the firmware.
WMI enumerates available data, methods, and events, and shepherds a
buffer of bits back and forth between firmware and userspace through
these methods.

Over the last few months, I've worked with Mario, Pali, Andy L, and
others to try and find an acceptable way to support Dell in their
efforts, while respecting Linux's design principles.

The first concession was to agree not to automatically publish a WMI
interface to userspace. This series includes the mechanism which
requires a GUID-specific driver to be written and to provide the file
ops structure, explicitly requesting the character device. This creates
a WMI GUID granular whitelist. I'm willing to deny any such driver which
is sent for review without the explicit collaboration of the vendor, to
ensure they are doing their due diligence with respect to their firmware
implementation. There has been some discussion in this thread regarding
Dell's firmware testing, and the access these interfaces have to
critical hardware resources.

The second concession was to acknowledge that some features implemented
in WMI are rightly the domain of the Linux kernel. LEDs, RFKill,
Backlight control, hotkey support, etc. When an exported GUID is used
for these purposes in addition to whatever purpose userspace requires
(see my comment above about a lack of functional boundaries in the
exported interfaces), we will provide a way to filter these usages. As
Mario said, we can either return an error, or we can attempt to handle
the request via the appropriate Linux kernel subsystem connections.

From the Linux kernel side, the ask is that we acknowledge that it is
not practical to audit every WMI interface to present a set of
functional knobs to userspace - because they were specifically designed
without that requirement.

The platform was designed in this way specifically to optimize the
software support effort on the part of the vendor. They don't write
Windows OS drivers for these mechanisms, it is unrealistic to expect
them to write them for Linux.

I believe we need to arrive at a compromise which allows a vendor to
work upstream like Mario is doing on behalf of Dell to fully enable
their platforms as they were designed on Linux, while ensuring we don't
adversely affect the functionality or security of other platforms. I
believe the whitelist and blacklist above provides for this compromise.

Perhaps an additional concession we should consider is to make exporting
WMI interfaces a configuration option. If not set, the drivers will
continue to work, but the WMI bus driver will not create the character
device, even if requested. This would allow vendors to create a fully
supported platform using upstream code, while allowing individuals the
control to disable WMI userspace functionality if they don't trust the
mechanism.

The alternative seems to be that we accept these features will not be
available on Linux for these platforms, or that vendors will develop a
single wmi mapping driver out of tree, likely without the noted
concessions.

1. https://msdn.microsoft.com/en-us/library/windows/hardware/dn614028(v=vs.85).aspx
   Specifically: "ACPI-to-WMI Mapper Goals for Windows Instrumentation"

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Greg KH]

On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote:
> On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote:
> > On Wed, 11 Oct 2017 11:27:36 -0500
> > Mario Limonciello <mario.limonciello@dell.com> wrote:
> > 
> > > There are some categories of tokens and SMBIOS calls that it makes
> > > sense to protect userspace from accessing.  These are calls that
> > > may write to one time use fields or activate hardware debugging
> > > capabilities.  They are not intended for general purpose use.
> > > 
> > > This same functionality may be be later extended to also intercept
> > > calls that may cause kernel functionality to get out of sync if
> > > the same functions are used by other drivers.
> > 
> > This doesn't work. You are creating an API. If you then have to remove
> > parts of the API because it messes stuff up you break your API guarantee.
> > 
> > As I said before, this needs to be a whitelist of stuff that is safe, and
> > it needs to have a security model. When you make a feature available you
> > can't nicely take it away again as you'll break people's user space.
> > 
> > Start with a whitelist of the ones you know people want to use, or that
> > your existing tooling you want to enable uses. Add others as needed in
> > future releases of the kernel.
> 
> Hi Alan,
> 
> I've attempted to get a focused discussion on this topic a few times,
> but have failed to get it the focus in really needs. Thanks for bringing
> it up. I look forward to your thoughts on the following:
> 
> The core of the issue is we are trying to achieve feature parity with
> Windows using the Windows Management Instrumentation (WMI) mechanism.
> I'm trying to support vendors like Dell in their attempts to do so, and
> acknowledging that this involves supporting a platform which was
> designed according to the norms and standards of the windows ecosystem.

Note, we really don't care what Windows does, as I really doubt they
care what we do, so this isn't a valid decision.  Do we also care about
OS-X and how it handles WMI?  :)

> This WMI mechanism was designed to enable vendors to create management
> tools which could talk to interfaces their firmware exposed for this
> purpose without any platform specific OS driver or vendor specific
> knowledge on the part of the OS driver [1].

That was how Microsoft designed it, due to totally different
requirements of their ecosystem.  You really can't compare the two for
obvious reasons, and I think that's the major disconnect here.

> The result of this design decision is as you would expect: there is no
> consistency in design, no guarantee of interfaces with functional
> boundaries, across the various WMI interface implementations across
> vendors (or even within the interface of a given vendor on a given
> platform).

See, different ecosystem, I like ours better :)

> The result is a mechanism which is fundamentally incompatible with
> standard Linux kernel approach to isolating userspace from the firmware.
> WMI enumerates available data, methods, and events, and shepherds a
> buffer of bits back and forth between firmware and userspace through
> these methods.
> 
> Over the last few months, I've worked with Mario, Pali, Andy L, and
> others to try and find an acceptable way to support Dell in their
> efforts, while respecting Linux's design principles.
> 
> The first concession was to agree not to automatically publish a WMI
> interface to userspace. This series includes the mechanism which
> requires a GUID-specific driver to be written and to provide the file
> ops structure, explicitly requesting the character device. This creates
> a WMI GUID granular whitelist. I'm willing to deny any such driver which
> is sent for review without the explicit collaboration of the vendor, to
> ensure they are doing their due diligence with respect to their firmware
> implementation. There has been some discussion in this thread regarding
> Dell's firmware testing, and the access these interfaces have to
> critical hardware resources.

How are you going to prevent out-of-tree drivers from using this same
api to directly get access to the WMI interface without using any type
of whitelist or review?

> The second concession was to acknowledge that some features implemented
> in WMI are rightly the domain of the Linux kernel. LEDs, RFKill,
> Backlight control, hotkey support, etc. When an exported GUID is used
> for these purposes in addition to whatever purpose userspace requires
> (see my comment above about a lack of functional boundaries in the
> exported interfaces), we will provide a way to filter these usages. As
> Mario said, we can either return an error, or we can attempt to handle
> the request via the appropriate Linux kernel subsystem connections.

That's great, but I don't see that in the patches posted, did I miss it?

> From the Linux kernel side, the ask is that we acknowledge that it is
> not practical to audit every WMI interface to present a set of
> functional knobs to userspace - because they were specifically designed
> without that requirement.

Not true, we can audit whatever we want.  whitelists are good for this
very reason.  Just because a vendor is used to replacing any part of the
OS with their own custom api direct to the hardware in other operating
systems, doesn't mean that we have to also emulate that crazy api as
well, especially given that it can be easily abused, as we discussed
before.

> The platform was designed in this way specifically to optimize the
> software support effort on the part of the vendor. They don't write
> Windows OS drivers for these mechanisms, it is unrealistic to expect
> them to write them for Linux.

Note, we write drivers for hardware for free if asked, so is it
unrealistic to require a kernel driver if we offer to do it for them? :)

> I believe we need to arrive at a compromise which allows a vendor to
> work upstream like Mario is doing on behalf of Dell to fully enable
> their platforms as they were designed on Linux, while ensuring we don't
> adversely affect the functionality or security of other platforms. I
> believe the whitelist and blacklist above provides for this compromise.
> 
> Perhaps an additional concession we should consider is to make exporting
> WMI interfaces a configuration option. If not set, the drivers will
> continue to work, but the WMI bus driver will not create the character
> device, even if requested. This would allow vendors to create a fully
> supported platform using upstream code, while allowing individuals the
> control to disable WMI userspace functionality if they don't trust the
> mechanism.

config options do not work, distros have to turn them all on, you know
that...

> The alternative seems to be that we accept these features will not be
> available on Linux for these platforms, or that vendors will develop a
> single wmi mapping driver out of tree, likely without the noted
> concessions.

So you are saying they are blackmailing us that if we do not provide
these wide-open api, then they just will take their toys and go home?
In other words, back where we are today?  :)

I understand the goal here of getting this to all work properly, but
note that this is a different type of an operating system, and for some
things, maybe we just do not allow direct userspace access for the
obvious reasons (security, maintance, auditability, long-term-support,
etc.)  A whitelist of known-good commands sounds like a good place to
start, why not start with that and go from there?

thanks,

greg k-h

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Pali Rohár]

On Friday 13 October 2017 11:43:14 Greg KH wrote:
> I understand the goal here of getting this to all work properly, but
> note that this is a different type of an operating system, and for some
> things, maybe we just do not allow direct userspace access for the
> obvious reasons (security, maintance, auditability, long-term-support,
> etc.)  A whitelist of known-good commands sounds like a good place to
> start, why not start with that and go from there?

So, I understood that Greg want to rather see explicit whitelist in
allowed functionality from userspace. And for such thing generic WMI API
is not a best option.

Creating generic API for kernel drivers for filtering WMI requests based
on some white list is hard.

If we want to choose this option, then it would be easier to create Dell
specific API for setting SMBIOS tokens and in kernel just add list of
whitelisted tokens, which userspace can set/unset.

Parsing SMBIOS requests in WMI buffer is harder, it leads to more
complicated code and basically serves same as "function specific API"
which is easier to implement and audit.

WMI is just "RPC" for function calls and inspecting it for security and
access is harder as designing new simple API which would mimic what is
"hidden" in that "RPC".

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

> Within Linux the security model is that items accessible through this interface
> are only accessible by root.

"root" has not been a security concept in the Linux kernel since about
2.0. If you are relying on file permissions then at best you are using
CAP_SYS_DAC which is too weak for this.

If you are allowing near unchecked communication with a third party
entity that the user doesn't trust too much you should be requiring
CAP_SYS_RAWIO.

In fact it's a fair argument hat if you require CAP_SYS_RAWIO and have a
module option you have to set to allow it that with the module loaded
with say

	insmod dell_smbios factory=1

does even blacklisted stuff then you are ok, because a process with
CAP_SYS_RAWIO has enough power to totally own the machine anyway
including taking over and doing the WMI call itself by hand in user space
or loading its own module.

Alan

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

> Within Linux the security model is that items accessible through this interface
> are only accessible by root.

"root" has not been a security concept in the Linux kernel since about
2.0. If you are relying on file permissions then at best you are using
CAP_SYS_DAC which is too weak for this.

If you are allowing near unchecked communication with a third party
entity that the user doesn't trust too much you should be requiring
CAP_SYS_RAWIO.

In fact it's a fair argument hat if you require CAP_SYS_RAWIO and have a
module option you have to set to allow it that with the module loaded
with say

	insmod dell_smbios factory=1

does even blacklisted stuff then you are ok, because a process with
CAP_SYS_RAWIO has enough power to totally own the machine anyway
including taking over and doing the WMI call itself by hand in user space
or loading its own module.

Alan

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, October 13, 2017 4:43 AM
> To: Darren Hart <dvhart@infradead.org>
> Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>;
> platform-driver-x86@vger.kernel.org; Andy Lutomirski <luto@kernel.org>;
> quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net;
> mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote:
> > On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote:
> > > On Wed, 11 Oct 2017 11:27:36 -0500
> > > Mario Limonciello <mario.limonciello@dell.com> wrote:
> > >
> > > > There are some categories of tokens and SMBIOS calls that it makes
> > > > sense to protect userspace from accessing.  These are calls that
> > > > may write to one time use fields or activate hardware debugging
> > > > capabilities.  They are not intended for general purpose use.
> > > >
> > > > This same functionality may be be later extended to also intercept
> > > > calls that may cause kernel functionality to get out of sync if
> > > > the same functions are used by other drivers.
> > >
> > > This doesn't work. You are creating an API. If you then have to remove
> > > parts of the API because it messes stuff up you break your API guarantee.
> > >
> > > As I said before, this needs to be a whitelist of stuff that is safe, and
> > > it needs to have a security model. When you make a feature available you
> > > can't nicely take it away again as you'll break people's user space.
> > >
> > > Start with a whitelist of the ones you know people want to use, or that
> > > your existing tooling you want to enable uses. Add others as needed in
> > > future releases of the kernel.
> >
> > Hi Alan,
> >
> > I've attempted to get a focused discussion on this topic a few times,
> > but have failed to get it the focus in really needs. Thanks for bringing
> > it up. I look forward to your thoughts on the following:
> >
> > The core of the issue is we are trying to achieve feature parity with
> > Windows using the Windows Management Instrumentation (WMI) mechanism.
> > I'm trying to support vendors like Dell in their attempts to do so, and
> > acknowledging that this involves supporting a platform which was
> > designed according to the norms and standards of the windows ecosystem.
> 
> Note, we really don't care what Windows does, as I really doubt they
> care what we do, so this isn't a valid decision.  Do we also care about
> OS-X and how it handles WMI?  :)

If you want to be able to offer tools that do the same thing on all OSes that
hardware ships with it's a good idea to do it through the same interface in
both places and to use the same code base on both if possible.  Otherwise
that's just asking for bugs.

Maybe Dell is alone in this area in trying to improve this, I don't know.

> 
> > This WMI mechanism was designed to enable vendors to create management
> > tools which could talk to interfaces their firmware exposed for this
> > purpose without any platform specific OS driver or vendor specific
> > knowledge on the part of the OS driver [1].
> 
> That was how Microsoft designed it, due to totally different
> requirements of their ecosystem.  You really can't compare the two for
> obvious reasons, and I think that's the major disconnect here.
> 
> > The result of this design decision is as you would expect: there is no
> > consistency in design, no guarantee of interfaces with functional
> > boundaries, across the various WMI interface implementations across
> > vendors (or even within the interface of a given vendor on a given
> > platform).
> 
> See, different ecosystem, I like ours better :)
> 
> > The result is a mechanism which is fundamentally incompatible with
> > standard Linux kernel approach to isolating userspace from the firmware.
> > WMI enumerates available data, methods, and events, and shepherds a
> > buffer of bits back and forth between firmware and userspace through
> > these methods.
> >
> > Over the last few months, I've worked with Mario, Pali, Andy L, and
> > others to try and find an acceptable way to support Dell in their
> > efforts, while respecting Linux's design principles.
> >
> > The first concession was to agree not to automatically publish a WMI
> > interface to userspace. This series includes the mechanism which
> > requires a GUID-specific driver to be written and to provide the file
> > ops structure, explicitly requesting the character device. This creates
> > a WMI GUID granular whitelist. I'm willing to deny any such driver which
> > is sent for review without the explicit collaboration of the vendor, to
> > ensure they are doing their due diligence with respect to their firmware
> > implementation. There has been some discussion in this thread regarding
> > Dell's firmware testing, and the access these interfaces have to
> > critical hardware resources.
> 
> How are you going to prevent out-of-tree drivers from using this same
> api to directly get access to the WMI interface without using any type
> of whitelist or review?

Out of tree drivers is really a distraction in this context.  An out of tree driver
could also just as easily create its own character device without using the
WMI bus.

> 
> > The second concession was to acknowledge that some features implemented
> > in WMI are rightly the domain of the Linux kernel. LEDs, RFKill,
> > Backlight control, hotkey support, etc. When an exported GUID is used
> > for these purposes in addition to whatever purpose userspace requires
> > (see my comment above about a lack of functional boundaries in the
> > exported interfaces), we will provide a way to filter these usages. As
> > Mario said, we can either return an error, or we can attempt to handle
> > the request via the appropriate Linux kernel subsystem connections.
> 
> That's great, but I don't see that in the patches posted, did I miss it?

You are correct, as of v7 this specific filtering of stuff already done in kernel 
is not present.  Unless there is more opposition to this approach I'll modify 
to return errors for those.

> 
> > From the Linux kernel side, the ask is that we acknowledge that it is
> > not practical to audit every WMI interface to present a set of
> > functional knobs to userspace - because they were specifically designed
> > without that requirement.
> 
> Not true, we can audit whatever we want.  whitelists are good for this
> very reason.  Just because a vendor is used to replacing any part of the
> OS with their own custom api direct to the hardware in other operating
> systems, doesn't mean that we have to also emulate that crazy api as
> well, especially given that it can be easily abused, as we discussed
> before.
> 
> > The platform was designed in this way specifically to optimize the
> > software support effort on the part of the vendor. They don't write
> > Windows OS drivers for these mechanisms, it is unrealistic to expect
> > them to write them for Linux.
> 
> Note, we write drivers for hardware for free if asked, so is it
> unrealistic to require a kernel driver if we offer to do it for them? :)
> 
> > I believe we need to arrive at a compromise which allows a vendor to
> > work upstream like Mario is doing on behalf of Dell to fully enable
> > their platforms as they were designed on Linux, while ensuring we don't
> > adversely affect the functionality or security of other platforms. I
> > believe the whitelist and blacklist above provides for this compromise.
> >
> > Perhaps an additional concession we should consider is to make exporting
> > WMI interfaces a configuration option. If not set, the drivers will
> > continue to work, but the WMI bus driver will not create the character
> > device, even if requested. This would allow vendors to create a fully
> > supported platform using upstream code, while allowing individuals the
> > control to disable WMI userspace functionality if they don't trust the
> > mechanism.
> 
> config options do not work, distros have to turn them all on, you know
> that...
> 
> > The alternative seems to be that we accept these features will not be
> > available on Linux for these platforms, or that vendors will develop a
> > single wmi mapping driver out of tree, likely without the noted
> > concessions.
> 
> So you are saying they are blackmailing us that if we do not provide
> these wide-open api, then they just will take their toys and go home?
> In other words, back where we are today?  :)

I think within all of this discussion the spirit of intent is getting lost.

There is an interface that exists today in Windows as a dell signed kernel 
driver and in Linux as dcdbas that lets various tools access interesting
pieces of data from the BIOS.

Dell is deprecating this interface and moving to a driverless model on 
Windows and this patch series represents the parity of the "driverless"
solution for Linux.

Certainly I can advocate internally that all this effort is a waste of time
and something can be carried out of tree and shipped with tools, but
I don't want people using our tools to have to use tainted kernels.

> 
> I understand the goal here of getting this to all work properly, but
> note that this is a different type of an operating system, and for some
> things, maybe we just do not allow direct userspace access for the
> obvious reasons (security, maintance, auditability, long-term-support,
> etc.)  A whitelist of known-good commands sounds like a good place to
> start, why not start with that and go from there?
> 

Take off your "kernel" hat and put on a "customer" hat for a few moments
while I try to put this in practical terms why the whitelist approach doesn't
scale for what I'm trying to do.

Let's say hypothetically a future version of this series that has only whitelisted
commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL
release etc.
Hardware coming out about that time works fine, you can control the various
knobs.  Then later that year some new headless hardware is released that has
zigbee controllers that work with inbox kernel drivers but you can't turn them 
on and off any way BUT through the manageability interface (it's headless!).
In the manageability interface we also offer a new class/select or token that 
can control the GPIO that turns on/off these radios.

A patch could be submitted upstream to whitelist that new class/select or token, 
but then you run into the problem that anyone on these stable LTS type distros
won't pick it up.  You can submit it to linux-stable and that might help Ubuntu, 
but a distro like RHEL doesn't track linux-stable.  So that means now you're stuck 
in a place that tools don't work with the interface solely because of a whitelist
that the kernel community hasn't yet blessed that it's ok to turn on/off zigbee
controllers through the OEM's manageability interface.

You now have introduced a ton of red tape that will artificially slow down supporting
HW with Linux for something as simple as controlling a GPIO in a manageability interface.

Tools that could otherwise work in many kernel versions will be gimped to only
offer full functionality in the latest kernel versions.

As a naïve customer why Does Dell's tool for managing my zigbee radios not work
in RHEL 7.x but only in RHEL 7.x+1?  No one will understand that.

So considering the above isn't offering stuff like this a decision better made by the OEM?
If the OEM  doen't want customers to be able to modify something we don't offer it in the
manageability interface.  If the kernel community doesn't want people to be 
modifying something the OEM does offer, it can just as well be blacklisted in the 
kernel driver like the current filtering approach offers.

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, October 13, 2017 4:43 AM
> To: Darren Hart <dvhart@infradead.org>
> Cc: Alan Cox <gnomes@lxorguk.ukuu.org.uk>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>;
> platform-driver-x86@vger.kernel.org; Andy Lutomirski <luto@kernel.org>;
> quasisec@google.com; pali.rohar@gmail.com; rjw@rjwysocki.net;
> mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote:
> > On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote:
> > > On Wed, 11 Oct 2017 11:27:36 -0500
> > > Mario Limonciello <mario.limonciello@dell.com> wrote:
> > >
> > > > There are some categories of tokens and SMBIOS calls that it makes
> > > > sense to protect userspace from accessing.  These are calls that
> > > > may write to one time use fields or activate hardware debugging
> > > > capabilities.  They are not intended for general purpose use.
> > > >
> > > > This same functionality may be be later extended to also intercept
> > > > calls that may cause kernel functionality to get out of sync if
> > > > the same functions are used by other drivers.
> > >
> > > This doesn't work. You are creating an API. If you then have to remove
> > > parts of the API because it messes stuff up you break your API guarantee.
> > >
> > > As I said before, this needs to be a whitelist of stuff that is safe, and
> > > it needs to have a security model. When you make a feature available you
> > > can't nicely take it away again as you'll break people's user space.
> > >
> > > Start with a whitelist of the ones you know people want to use, or that
> > > your existing tooling you want to enable uses. Add others as needed in
> > > future releases of the kernel.
> >
> > Hi Alan,
> >
> > I've attempted to get a focused discussion on this topic a few times,
> > but have failed to get it the focus in really needs. Thanks for bringing
> > it up. I look forward to your thoughts on the following:
> >
> > The core of the issue is we are trying to achieve feature parity with
> > Windows using the Windows Management Instrumentation (WMI) mechanism.
> > I'm trying to support vendors like Dell in their attempts to do so, and
> > acknowledging that this involves supporting a platform which was
> > designed according to the norms and standards of the windows ecosystem.
> 
> Note, we really don't care what Windows does, as I really doubt they
> care what we do, so this isn't a valid decision.  Do we also care about
> OS-X and how it handles WMI?  :)

If you want to be able to offer tools that do the same thing on all OSes that
hardware ships with it's a good idea to do it through the same interface in
both places and to use the same code base on both if possible.  Otherwise
that's just asking for bugs.

Maybe Dell is alone in this area in trying to improve this, I don't know.

> 
> > This WMI mechanism was designed to enable vendors to create management
> > tools which could talk to interfaces their firmware exposed for this
> > purpose without any platform specific OS driver or vendor specific
> > knowledge on the part of the OS driver [1].
> 
> That was how Microsoft designed it, due to totally different
> requirements of their ecosystem.  You really can't compare the two for
> obvious reasons, and I think that's the major disconnect here.
> 
> > The result of this design decision is as you would expect: there is no
> > consistency in design, no guarantee of interfaces with functional
> > boundaries, across the various WMI interface implementations across
> > vendors (or even within the interface of a given vendor on a given
> > platform).
> 
> See, different ecosystem, I like ours better :)
> 
> > The result is a mechanism which is fundamentally incompatible with
> > standard Linux kernel approach to isolating userspace from the firmware.
> > WMI enumerates available data, methods, and events, and shepherds a
> > buffer of bits back and forth between firmware and userspace through
> > these methods.
> >
> > Over the last few months, I've worked with Mario, Pali, Andy L, and
> > others to try and find an acceptable way to support Dell in their
> > efforts, while respecting Linux's design principles.
> >
> > The first concession was to agree not to automatically publish a WMI
> > interface to userspace. This series includes the mechanism which
> > requires a GUID-specific driver to be written and to provide the file
> > ops structure, explicitly requesting the character device. This creates
> > a WMI GUID granular whitelist. I'm willing to deny any such driver which
> > is sent for review without the explicit collaboration of the vendor, to
> > ensure they are doing their due diligence with respect to their firmware
> > implementation. There has been some discussion in this thread regarding
> > Dell's firmware testing, and the access these interfaces have to
> > critical hardware resources.
> 
> How are you going to prevent out-of-tree drivers from using this same
> api to directly get access to the WMI interface without using any type
> of whitelist or review?

Out of tree drivers is really a distraction in this context.  An out of tree driver
could also just as easily create its own character device without using the
WMI bus.

> 
> > The second concession was to acknowledge that some features implemented
> > in WMI are rightly the domain of the Linux kernel. LEDs, RFKill,
> > Backlight control, hotkey support, etc. When an exported GUID is used
> > for these purposes in addition to whatever purpose userspace requires
> > (see my comment above about a lack of functional boundaries in the
> > exported interfaces), we will provide a way to filter these usages. As
> > Mario said, we can either return an error, or we can attempt to handle
> > the request via the appropriate Linux kernel subsystem connections.
> 
> That's great, but I don't see that in the patches posted, did I miss it?

You are correct, as of v7 this specific filtering of stuff already done in kernel 
is not present.  Unless there is more opposition to this approach I'll modify 
to return errors for those.

> 
> > From the Linux kernel side, the ask is that we acknowledge that it is
> > not practical to audit every WMI interface to present a set of
> > functional knobs to userspace - because they were specifically designed
> > without that requirement.
> 
> Not true, we can audit whatever we want.  whitelists are good for this
> very reason.  Just because a vendor is used to replacing any part of the
> OS with their own custom api direct to the hardware in other operating
> systems, doesn't mean that we have to also emulate that crazy api as
> well, especially given that it can be easily abused, as we discussed
> before.
> 
> > The platform was designed in this way specifically to optimize the
> > software support effort on the part of the vendor. They don't write
> > Windows OS drivers for these mechanisms, it is unrealistic to expect
> > them to write them for Linux.
> 
> Note, we write drivers for hardware for free if asked, so is it
> unrealistic to require a kernel driver if we offer to do it for them? :)
> 
> > I believe we need to arrive at a compromise which allows a vendor to
> > work upstream like Mario is doing on behalf of Dell to fully enable
> > their platforms as they were designed on Linux, while ensuring we don't
> > adversely affect the functionality or security of other platforms. I
> > believe the whitelist and blacklist above provides for this compromise.
> >
> > Perhaps an additional concession we should consider is to make exporting
> > WMI interfaces a configuration option. If not set, the drivers will
> > continue to work, but the WMI bus driver will not create the character
> > device, even if requested. This would allow vendors to create a fully
> > supported platform using upstream code, while allowing individuals the
> > control to disable WMI userspace functionality if they don't trust the
> > mechanism.
> 
> config options do not work, distros have to turn them all on, you know
> that...
> 
> > The alternative seems to be that we accept these features will not be
> > available on Linux for these platforms, or that vendors will develop a
> > single wmi mapping driver out of tree, likely without the noted
> > concessions.
> 
> So you are saying they are blackmailing us that if we do not provide
> these wide-open api, then they just will take their toys and go home?
> In other words, back where we are today?  :)

I think within all of this discussion the spirit of intent is getting lost.

There is an interface that exists today in Windows as a dell signed kernel 
driver and in Linux as dcdbas that lets various tools access interesting
pieces of data from the BIOS.

Dell is deprecating this interface and moving to a driverless model on 
Windows and this patch series represents the parity of the "driverless"
solution for Linux.

Certainly I can advocate internally that all this effort is a waste of time
and something can be carried out of tree and shipped with tools, but
I don't want people using our tools to have to use tainted kernels.

> 
> I understand the goal here of getting this to all work properly, but
> note that this is a different type of an operating system, and for some
> things, maybe we just do not allow direct userspace access for the
> obvious reasons (security, maintance, auditability, long-term-support,
> etc.)  A whitelist of known-good commands sounds like a good place to
> start, why not start with that and go from there?
> 

Take off your "kernel" hat and put on a "customer" hat for a few moments
while I try to put this in practical terms why the whitelist approach doesn't
scale for what I'm trying to do.

Let's say hypothetically a future version of this series that has only whitelisted
commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL
release etc.
Hardware coming out about that time works fine, you can control the various
knobs.  Then later that year some new headless hardware is released that has
zigbee controllers that work with inbox kernel drivers but you can't turn them 
on and off any way BUT through the manageability interface (it's headless!).
In the manageability interface we also offer a new class/select or token that 
can control the GPIO that turns on/off these radios.

A patch could be submitted upstream to whitelist that new class/select or token, 
but then you run into the problem that anyone on these stable LTS type distros
won't pick it up.  You can submit it to linux-stable and that might help Ubuntu, 
but a distro like RHEL doesn't track linux-stable.  So that means now you're stuck 
in a place that tools don't work with the interface solely because of a whitelist
that the kernel community hasn't yet blessed that it's ok to turn on/off zigbee
controllers through the OEM's manageability interface.

You now have introduced a ton of red tape that will artificially slow down supporting
HW with Linux for something as simple as controlling a GPIO in a manageability interface.

Tools that could otherwise work in many kernel versions will be gimped to only
offer full functionality in the latest kernel versions.

As a naïve customer why Does Dell's tool for managing my zigbee radios not work
in RHEL 7.x but only in RHEL 7.x+1?  No one will understand that.

So considering the above isn't offering stuff like this a decision better made by the OEM?
If the OEM  doen't want customers to be able to modify something we don't offer it in the
manageability interface.  If the kernel community doesn't want people to be 
modifying something the OEM does offer, it can just as well be blacklisted in the 
kernel driver like the current filtering approach offers.

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

On Fri, 13 Oct 2017 15:03:10 +0000
> Take off your "kernel" hat and put on a "customer" hat for a few moments
> while I try to put this in practical terms why the whitelist approach doesn't
> scale for what I'm trying to do.

As a customer I'm more worried about someone trashing my system or
breaking my security.

> So considering the above isn't offering stuff like this a decision better made by the OEM?
> If the OEM  doen't want customers to be able to modify something we don't offer it in the
> manageability interface.  If the kernel community doesn't want people to be 
> modifying something the OEM does offer, it can just as well be blacklisted in the 
> kernel driver like the current filtering approach offers.

So you implement the rule

	if (whitelisted & (capabilities && whitelist->capability_need) ==
	whitelist->capability_need))
		return ALLOWED;

	if (capable(CAP_SYS_RAWIO))
		return ALLOWED;

	return NO

This puts you in the position where - known tools work and can sometimes
be unprivileged. Privileged tools with enough priv to screw the machien
can work anyway. Which is better than the starting point


You could further enhance this by having a CAP_SYS_RAWIO interface to add
whitelist entries, or to add an eBPF filter that can also make decisions
for you.

Now you've got the ability to push a policy update.

Alan

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

On Fri, 13 Oct 2017 15:03:10 +0000
> Take off your "kernel" hat and put on a "customer" hat for a few moments
> while I try to put this in practical terms why the whitelist approach doesn't
> scale for what I'm trying to do.

As a customer I'm more worried about someone trashing my system or
breaking my security.

> So considering the above isn't offering stuff like this a decision better made by the OEM?
> If the OEM  doen't want customers to be able to modify something we don't offer it in the
> manageability interface.  If the kernel community doesn't want people to be 
> modifying something the OEM does offer, it can just as well be blacklisted in the 
> kernel driver like the current filtering approach offers.

So you implement the rule

	if (whitelisted & (capabilities && whitelist->capability_need) ==
	whitelist->capability_need))
		return ALLOWED;

	if (capable(CAP_SYS_RAWIO))
		return ALLOWED;

	return NO

This puts you in the position where - known tools work and can sometimes
be unprivileged. Privileged tools with enough priv to screw the machien
can work anyway. Which is better than the starting point


You could further enhance this by having a CAP_SYS_RAWIO interface to add
whitelist entries, or to add an eBPF filter that can also make decisions
for you.

Now you've got the ability to push a policy update.

Alan

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk]
> Sent: Friday, October 13, 2017 10:20 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: greg@kroah.com; dvhart@infradead.org; andy.shevchenko@gmail.com;
> linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> luto@kernel.org; quasisec@google.com; pali.rohar@gmail.com;
> rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Fri, 13 Oct 2017 15:03:10 +0000
> > Take off your "kernel" hat and put on a "customer" hat for a few moments
> > while I try to put this in practical terms why the whitelist approach doesn't
> > scale for what I'm trying to do.
> 
> As a customer I'm more worried about someone trashing my system or
> breaking my security.
> 
> > So considering the above isn't offering stuff like this a decision better made by
> the OEM?
> > If the OEM  doen't want customers to be able to modify something we don't
> offer it in the
> > manageability interface.  If the kernel community doesn't want people to be
> > modifying something the OEM does offer, it can just as well be blacklisted in
> the
> > kernel driver like the current filtering approach offers.
> 
> So you implement the rule
> 
> 	if (whitelisted & (capabilities && whitelist->capability_need) ==
> 	whitelist->capability_need))
> 		return ALLOWED;
> 
> 	if (capable(CAP_SYS_RAWIO))
> 		return ALLOWED;
> 
> 	return NO
> 
> This puts you in the position where - known tools work and can sometimes
> be unprivileged. Privileged tools with enough priv to screw the machien
> can work anyway. Which is better than the starting point
> 
> 
> You could further enhance this by having a CAP_SYS_RAWIO interface to add
> whitelist entries, or to add an eBPF filter that can also make decisions
> for you.
> 
> Now you've got the ability to push a policy update.
> 
> Alan

Thanks for this idea, I think it's productive in working towards a solution.

I'll give it some more thought on what items I feel should be whitelisted to
unprivileged processes.  I feel like the number of entries that match this will
be fairly low.

I think I'd actually like to meld this with your other ideas and what I've 
currently got.  What do you think of this approach:

	/* kernel community doesn't feel userspace should have access at all
	  * or other kernel drivers use this
	  */
	if (blacklisted)
		return NO;

	/* unprivileged access allowed */
 	if (whitelisted & (capabilities && whitelist->capability_need) ==
 	whitelist->capability_need))
 		return ALLOWED;
 
	/* not yet in whitelist, or need privs to do */
 	if (capable(CAP_SYS_RAWIO))
 		return ALLOWED;
 
 	return NO

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Alan Cox [mailto:gnomes@lxorguk.ukuu.org.uk]
> Sent: Friday, October 13, 2017 10:20 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: greg@kroah.com; dvhart@infradead.org; andy.shevchenko@gmail.com;
> linux-kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> luto@kernel.org; quasisec@google.com; pali.rohar@gmail.com;
> rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> On Fri, 13 Oct 2017 15:03:10 +0000
> > Take off your "kernel" hat and put on a "customer" hat for a few moments
> > while I try to put this in practical terms why the whitelist approach doesn't
> > scale for what I'm trying to do.
> 
> As a customer I'm more worried about someone trashing my system or
> breaking my security.
> 
> > So considering the above isn't offering stuff like this a decision better made by
> the OEM?
> > If the OEM  doen't want customers to be able to modify something we don't
> offer it in the
> > manageability interface.  If the kernel community doesn't want people to be
> > modifying something the OEM does offer, it can just as well be blacklisted in
> the
> > kernel driver like the current filtering approach offers.
> 
> So you implement the rule
> 
> 	if (whitelisted & (capabilities && whitelist->capability_need) ==
> 	whitelist->capability_need))
> 		return ALLOWED;
> 
> 	if (capable(CAP_SYS_RAWIO))
> 		return ALLOWED;
> 
> 	return NO
> 
> This puts you in the position where - known tools work and can sometimes
> be unprivileged. Privileged tools with enough priv to screw the machien
> can work anyway. Which is better than the starting point
> 
> 
> You could further enhance this by having a CAP_SYS_RAWIO interface to add
> whitelist entries, or to add an eBPF filter that can also make decisions
> for you.
> 
> Now you've got the ability to push a policy update.
> 
> Alan

Thanks for this idea, I think it's productive in working towards a solution.

I'll give it some more thought on what items I feel should be whitelisted to
unprivileged processes.  I feel like the number of entries that match this will
be fairly low.

I think I'd actually like to meld this with your other ideas and what I've 
currently got.  What do you think of this approach:

	/* kernel community doesn't feel userspace should have access at all
	  * or other kernel drivers use this
	  */
	if (blacklisted)
		return NO;

	/* unprivileged access allowed */
 	if (whitelisted & (capabilities && whitelist->capability_need) ==
 	whitelist->capability_need))
 		return ALLOWED;
 
	/* not yet in whitelist, or need privs to do */
 	if (capable(CAP_SYS_RAWIO))
 		return ALLOWED;
 
 	return NO

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Greg KH]

Just want to respond to this part first:

On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote:
> Take off your "kernel" hat and put on a "customer" hat for a few moments
> while I try to put this in practical terms why the whitelist approach doesn't
> scale for what I'm trying to do.

Heh, you do know my background in running an enterprise kernel team,
right? :)

> Let's say hypothetically a future version of this series that has only whitelisted
> commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL
> release etc.
> Hardware coming out about that time works fine, you can control the various
> knobs.  Then later that year some new headless hardware is released that has
> zigbee controllers that work with inbox kernel drivers but you can't turn them 
> on and off any way BUT through the manageability interface (it's headless!).
> In the manageability interface we also offer a new class/select or token that 
> can control the GPIO that turns on/off these radios.

It's the "later" that you are missing here.  We only have code today for
hardware we have today.  If you come out with new hardware, you need new
kernel drivers for it, and as such, "old" enterprise kernels will just
not work properly.

It's always been that way, this is nothing new, we can't predict the
future, and is one big reason why I think the whole "enterprise" distro
market is wrong and going to fail in the end :)

Same goes for that new device id for the wifi chip, or the video camera
or the fingerprint reader.  Those have to be added to the kernel, and if
the distro so desires, backported to their old and crufty version.

This has been happening for two decades now, somehow coming up with a
"raw" pipe from userspace to the kernel to control the hardware just
because you don't want to update the kernel code, isn't going to solve
the issue here (hint, you now have to update your userspace code, why is
that suddenly easier than the kernel?)

I know hardware companies want to stop writing software for their new
hardware designs.  I too want a pony :)

Sorry, this argument isn't going to fly.

greg k-h

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Darren Hart]

On Fri, Oct 13, 2017 at 11:43:14AM +0200, Greg KH wrote:

Hi Greg, first off - thanks for the response and the time you spent on
it. My reply is longer than I'd like, but I'm not having much luck
trimming it down without omitting things I think need to be noted. And
as I try, the thread keeps getting longer. Ahhhhh :-) So I'm sending
this now, and will continue with the rest of the thread. Yikes.

> On Thu, Oct 12, 2017 at 05:46:45PM -0700, Darren Hart wrote:
> > On Thu, Oct 12, 2017 at 11:09:03AM +0100, One Thousand Gnomes wrote:
> > > On Wed, 11 Oct 2017 11:27:36 -0500
> > > Mario Limonciello <mario.limonciello@dell.com> wrote:
> > > 
> > > > There are some categories of tokens and SMBIOS calls that it makes
> > > > sense to protect userspace from accessing.  These are calls that
> > > > may write to one time use fields or activate hardware debugging
> > > > capabilities.  They are not intended for general purpose use.
> > > > 
> > > > This same functionality may be be later extended to also intercept
> > > > calls that may cause kernel functionality to get out of sync if
> > > > the same functions are used by other drivers.
> > > 
> > > This doesn't work. You are creating an API. If you then have to remove
> > > parts of the API because it messes stuff up you break your API guarantee.
> > > 
> > > As I said before, this needs to be a whitelist of stuff that is safe, and
> > > it needs to have a security model. When you make a feature available you
> > > can't nicely take it away again as you'll break people's user space.
> > > 
> > > Start with a whitelist of the ones you know people want to use, or that
> > > your existing tooling you want to enable uses. Add others as needed in
> > > future releases of the kernel.
> > 
> > Hi Alan,
> > 
> > I've attempted to get a focused discussion on this topic a few times,
> > but have failed to get it the focus in really needs. Thanks for bringing
> > it up. I look forward to your thoughts on the following:
> > 
> > The core of the issue is we are trying to achieve feature parity with
> > Windows using the Windows Management Instrumentation (WMI) mechanism.
> > I'm trying to support vendors like Dell in their attempts to do so, and
> > acknowledging that this involves supporting a platform which was
> > designed according to the norms and standards of the windows ecosystem.
> 
> Note, we really don't care what Windows does, as I really doubt they
> care what we do, so this isn't a valid decision.  Do we also care about
> OS-X and how it handles WMI?  :)
> 
> > This WMI mechanism was designed to enable vendors to create management
> > tools which could talk to interfaces their firmware exposed for this
> > purpose without any platform specific OS driver or vendor specific
> > knowledge on the part of the OS driver [1].
> 
> That was how Microsoft designed it, due to totally different
> requirements of their ecosystem.  You really can't compare the two for
> obvious reasons, and I think that's the major disconnect here.
> 
> > The result of this design decision is as you would expect: there is no
> > consistency in design, no guarantee of interfaces with functional
> > boundaries, across the various WMI interface implementations across
> > vendors (or even within the interface of a given vendor on a given
> > platform).
> 
> See, different ecosystem, I like ours better :)
> 

I'm not sure if you're agreeing with me or arguing with me :-)

The point I'm trying to make is that the this platform (and most laptops
and PCs) was designed for the windows ecosystem, and is part of that
ecosystem. They are very different, as you say above, and that means
that it isn't reasonable to expect that platform to fit in nicely with
the Linux ecosystem when it was designed for a very different one. I
think that needs to be acknowledged as we try to support these
platforms. Round hole, square peg... legos and tinker toys... whatever
metaphor works.

> > The result is a mechanism which is fundamentally incompatible with
> > standard Linux kernel approach to isolating userspace from the firmware.
> > WMI enumerates available data, methods, and events, and shepherds a
> > buffer of bits back and forth between firmware and userspace through
> > these methods.
> > 
> > Over the last few months, I've worked with Mario, Pali, Andy L, and
> > others to try and find an acceptable way to support Dell in their
> > efforts, while respecting Linux's design principles.
> > 
> > The first concession was to agree not to automatically publish a WMI
> > interface to userspace. This series includes the mechanism which
> > requires a GUID-specific driver to be written and to provide the file
> > ops structure, explicitly requesting the character device. This creates
> > a WMI GUID granular whitelist. I'm willing to deny any such driver which
> > is sent for review without the explicit collaboration of the vendor, to
> > ensure they are doing their due diligence with respect to their firmware
> > implementation. There has been some discussion in this thread regarding
> > Dell's firmware testing, and the access these interfaces have to
> > critical hardware resources.
> 
> How are you going to prevent out-of-tree drivers from using this same
> api to directly get access to the WMI interface without using any type
> of whitelist or review?

I don't think that's the right question. The interface we're proposing
here is more complex than a simple wmi-mapping driver would be. If
someone were to create an out of tree driver, I suspect they would start
by writing that, and the rest is moot. Map all WMI calls to userspace,
export the MOF, done. Just like Windows. No need to write any platform
specific drivers at all. I'm not saying it's right or even a good idea,
but it's the shortest path to do what they want, and it's consistent
with what they already do, and would simplify the porting of the
userspace management applications.

> > The second concession was to acknowledge that some features implemented
> > in WMI are rightly the domain of the Linux kernel. LEDs, RFKill,
> > Backlight control, hotkey support, etc. When an exported GUID is used
> > for these purposes in addition to whatever purpose userspace requires
> > (see my comment above about a lack of functional boundaries in the
> > exported interfaces), we will provide a way to filter these usages. As
> > Mario said, we can either return an error, or we can attempt to handle
> > the request via the appropriate Linux kernel subsystem connections.
> 
> That's great, but I don't see that in the patches posted, did I miss it?
> 

New in v7. Patch 10/15 (this one). I would expect to see more of this in
the other and future *-wmi drivers where a MOF is available and methods
are used for both Linux internal things (LEDs, hotkeys) and for
management tasks. Hopefully we don't see a lot of those oversubscribed
GUIDs, but nothing in the spec discourages that - which means we'll see
more than we'd like I'm sure :-(

> > From the Linux kernel side, the ask is that we acknowledge that it is
> > not practical to audit every WMI interface to present a set of
> > functional knobs to userspace - because they were specifically designed
> > without that requirement.
> 
> Not true, we can audit whatever we want.  whitelists are good for this
> very reason.  Just because a vendor is used to replacing any part of the
> OS with their own custom api direct to the hardware in other operating
> systems, doesn't mean that we have to also emulate that crazy api as
> well, especially given that it can be easily abused, as we discussed
> before.

It's technically feasible, sure. My argument is that it isn't practical.
Meaning vendors won't do it. It's actively contrary to the intent of the
WMI mechanism, which means the interfaces won't have been designed to
facilitate audit by the OS, and these audits have the potential to turn
into incredibly complex bit checks. I discussed this previously here:

https://lkml.org/lkml/2017/6/13/147

| > > And filter layer which will accept only WMI calls which are safe for
| > > currently loaded/used kernel modules seems like a sane idea to ensure
| > > functionality of kernel plus allow userspace to do other things.
| >
| > My biggest concern with this approach is maintenance. Because we would be doing
| > something unforeseen by the specification, the various vendor implemented WMI
| > APIs are not likely to be amenable to filtering. I can see these filters
| > getting extremely complicated. They are "high touch", by which I mean each
| > generation of platform may require subtle tweaks, which will be difficult to
| > verify they don't break past generations.
|
| Agreed.  As mentioned before I think the only sensible approach is
| white listing GUIDs that have a valid userspace use case.  And use
| the dynamic IDs approach to add them for debugging and reverse
| engineering.

But, see the link for Christoph's full take on the subject :-)

> > The platform was designed in this way specifically to optimize the
> > software support effort on the part of the vendor. They don't write
> > Windows OS drivers for these mechanisms, it is unrealistic to expect
> > them to write them for Linux.
> 
> Note, we write drivers for hardware for free if asked, so is it
> unrealistic to require a kernel driver if we offer to do it for them? :)

In my opinion, yes, it is. The system was designed to be driver-less. No
company is going to place their product delivery schedule in the hands
of an unpaid contractor who gets to dictate to them how their platform
should be designed from the perspective of an OS with a tiny fraction of
their install base. This is fundamentally different from how their
entire organization is structured, and it's unrealistic to expect them
to change that to support Linux on a laptop. Even if there were a few
success stories, the model obviously can't scale. Vendors need to be
able to design and implement their platform support on their own
schedule and as independently as possible.

Note, from the same link above, Christoph disagrees with me strongly on
this point:

| Hell no!  The last thing we need on Linux is systems that once support
| us added don't just work out of the box because you're missing your
| vendor blob.

Note that Mario has already posted links to the sources for the tools
interacting with this interface, and the previous mechanism (libsmbios)
source have also been publicly available. Perhaps we continue to make
this a requirement to expose WMI features - an open source userspace
client which uses them (seems we do that for other interfaces anyway).

What I would tell Intel teams when trying to get them to work upstream
to enable customers to build designs with their products was: Enable
them to enable themselves, and get out of their way. I think a similar
approach is required here.

> > I believe we need to arrive at a compromise which allows a vendor to
> > work upstream like Mario is doing on behalf of Dell to fully enable
> > their platforms as they were designed on Linux, while ensuring we don't
> > adversely affect the functionality or security of other platforms. I
> > believe the whitelist and blacklist above provides for this compromise.
> > 
> > Perhaps an additional concession we should consider is to make exporting
> > WMI interfaces a configuration option. If not set, the drivers will
> > continue to work, but the WMI bus driver will not create the character
> > device, even if requested. This would allow vendors to create a fully
> > supported platform using upstream code, while allowing individuals the
> > control to disable WMI userspace functionality if they don't trust the
> > mechanism.
> 
> config options do not work, distros have to turn them all on, you know
> that...
> 

As I understand it, the distros will also take out of tree drivers and
patches to support hardware platforms for partners, so I think that
battle is already lost. I'd much rather have them integrating upstream
code we have some influence over.

If a CONFIG option is not acceptable, perhaps a runtime sysfs toggle?

Or something like the factory=1 module parameter Alan mentioned to
Mario.

> > The alternative seems to be that we accept these features will not be
> > available on Linux for these platforms, or that vendors will develop a
> > single wmi mapping driver out of tree, likely without the noted
> > concessions.
> 
> So you are saying they are blackmailing us that if we do not provide
> these wide-open api, then they just will take their toys and go home?
> In other words, back where we are today?  :)
> 

That's a fun sound byte, but not what I said. ;-) I'm trying to address
a practical reality. We're at a disadvantage because we're a minority OS
for these types of systems. I think we'd all like stronger support for
these platforms, and getting the vendors involved will help that.

If all they have to do on Windows to support these features is update
their management application, while on Linux they have to write a driver
for every platform and GUID, and on top of that perform an audit in the
kernel driver - all of which pushes more high maintenance platform
specific knowledge into the Linux kernel... (effort which duplicates
whatever they have done within the platform firmware already) I just
don't believe any vendor will be able to justify the expense.

> I understand the goal here of getting this to all work properly, but
> note that this is a different type of an operating system, and for some
> things, maybe we just do not allow direct userspace access for the
> obvious reasons (security, maintance, auditability, long-term-support,
> etc.)  A whitelist of known-good commands sounds like a good place to
> start, why not start with that and go from there?

This was my initial position on this as well. A clearly defined
functional interface from firmware which we can pick and choose how to
export to userspace would be the ideal. Unfortunately, that just isn't
what WMI is. As I mentioned above, at its core, WMI methods accept an
arbitrarily sized bucket of bits. The notion of a "command" doesn't
exist in the specification. I'm also not confident we can be sure these
interfaces don't change over time from platform to platform (just
another possible artifact from the design and intent of WMI).

I forgot to mention the third concession yesterday. For future drivers
for which the firmware provides a MOF (this one doesn't), we plan to do
the MOF parsing within the kernel instead of just in userspace to
provide some level of automated audit by the WMI subsystem - but it
isn't yet clear to me how useful that will really be (suspect we can
check types, but not values or ranges).

I understand our (kernel maintainers') desire to mediate between
userspace and firmware. I'm wondering if Alan's point about
CAP_SYS_RAWIO is something that needs more consideration. Windows has a
number of "namespace access settings" which limit the use of WMI,
perhaps this would provide a similar parallel?

https://msdn.microsoft.com/en-us/library/aa822575(v=vs.85).aspx

I also understand the vendors' position of owning their platforms and
using a common mechanism across OSes. In this case, and given the level
of involvement from Dell in particular and their publication of the
userspace tools and the whitelist and blacklist concessions, I think the
right thing, and the only practical way to see WMI supported through
upstream code, is to allow for WMI to be implemented in Linux in a way
that is compatible with how they designed their platforms.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, October 13, 2017 10:56 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; gnomes@lxorguk.ukuu.org.uk;
> andy.shevchenko@gmail.com; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; luto@kernel.org; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> Just want to respond to this part first:
> 
> On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote:
> > Take off your "kernel" hat and put on a "customer" hat for a few moments
> > while I try to put this in practical terms why the whitelist approach doesn't
> > scale for what I'm trying to do.
> 
> Heh, you do know my background in running an enterprise kernel team,
> right? :)
> 
> > Let's say hypothetically a future version of this series that has only whitelisted
> > commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL
> > release etc.
> > Hardware coming out about that time works fine, you can control the various
> > knobs.  Then later that year some new headless hardware is released that has
> > zigbee controllers that work with inbox kernel drivers but you can't turn them
> > on and off any way BUT through the manageability interface (it's headless!).
> > In the manageability interface we also offer a new class/select or token that
> > can control the GPIO that turns on/off these radios.
> 
> It's the "later" that you are missing here.  We only have code today for
> hardware we have today.  If you come out with new hardware, you need new
> kernel drivers for it, and as such, "old" enterprise kernels will just
> not work properly.
> 
> It's always been that way, this is nothing new, we can't predict the
> future, and is one big reason why I think the whole "enterprise" distro
> market is wrong and going to fail in the end :)
> 
> Same goes for that new device id for the wifi chip, or the video camera
> or the fingerprint reader.  Those have to be added to the kernel, and if
> the distro so desires, backported to their old and crufty version.
> 

There's plenty of cases you're absolutely right and I have a hate/hate relationship
regarding in this area.

I was very careful in my example to pick something that it's just the manageability
interface that changes.

> This has been happening for two decades now, somehow coming up with a
> "raw" pipe from userspace to the kernel to control the hardware just
> because you don't want to update the kernel code, isn't going to solve
> the issue here (hint, you now have to update your userspace code, why is
> that suddenly easier than the kernel?)
> 

It's easier because you can control your own destiny.

Changing the kernel code means submitting something upstream, 
submitting something to -stable, convincing the enterprise distro folks
to pull in your fix, adding code to your tool to test whether the kernel
supports your newly whitelisted function adding support for this function
to the tool and then posting that tool somewhere for consumption.

Changing userspace code means changing userspace code, posting it
somewhere and you're done.

As Darren mentioned most companies are built around large organizations
with specific touch points.  The people who decide what the next features
will be for manageability aren't the same people who write the tools, who
aren't the same people that implement them in the BIOS and aren't the 
same people that would be submitting any of this into the kernel.  Asking
every new option to be whitelisted in the kernel and getting that translated
into upstream, into stable, into distros, updating tools for etc is a giant process
undertaking that companies will look at and say uh why?

I think Alan's suggestions are leading this the right way though, whitelisting
for certain unprivileged actions, blacklisting certain actions, looking for capabilities
but still letting tools with the right capabilities or permissions do what they
want to do.  It's a balancing act.

RE: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Friday, October 13, 2017 10:56 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; gnomes@lxorguk.ukuu.org.uk;
> andy.shevchenko@gmail.com; linux-kernel@vger.kernel.org; platform-driver-
> x86@vger.kernel.org; luto@kernel.org; quasisec@google.com;
> pali.rohar@gmail.com; rjw@rjwysocki.net; mjg59@google.com; hch@lst.de
> Subject: Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability
> for requests
> 
> Just want to respond to this part first:
> 
> On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote:
> > Take off your "kernel" hat and put on a "customer" hat for a few moments
> > while I try to put this in practical terms why the whitelist approach doesn't
> > scale for what I'm trying to do.
> 
> Heh, you do know my background in running an enterprise kernel team,
> right? :)
> 
> > Let's say hypothetically a future version of this series that has only whitelisted
> > commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL
> > release etc.
> > Hardware coming out about that time works fine, you can control the various
> > knobs.  Then later that year some new headless hardware is released that has
> > zigbee controllers that work with inbox kernel drivers but you can't turn them
> > on and off any way BUT through the manageability interface (it's headless!).
> > In the manageability interface we also offer a new class/select or token that
> > can control the GPIO that turns on/off these radios.
> 
> It's the "later" that you are missing here.  We only have code today for
> hardware we have today.  If you come out with new hardware, you need new
> kernel drivers for it, and as such, "old" enterprise kernels will just
> not work properly.
> 
> It's always been that way, this is nothing new, we can't predict the
> future, and is one big reason why I think the whole "enterprise" distro
> market is wrong and going to fail in the end :)
> 
> Same goes for that new device id for the wifi chip, or the video camera
> or the fingerprint reader.  Those have to be added to the kernel, and if
> the distro so desires, backported to their old and crufty version.
> 

There's plenty of cases you're absolutely right and I have a hate/hate relationship
regarding in this area.

I was very careful in my example to pick something that it's just the manageability
interface that changes.

> This has been happening for two decades now, somehow coming up with a
> "raw" pipe from userspace to the kernel to control the hardware just
> because you don't want to update the kernel code, isn't going to solve
> the issue here (hint, you now have to update your userspace code, why is
> that suddenly easier than the kernel?)
> 

It's easier because you can control your own destiny.

Changing the kernel code means submitting something upstream, 
submitting something to -stable, convincing the enterprise distro folks
to pull in your fix, adding code to your tool to test whether the kernel
supports your newly whitelisted function adding support for this function
to the tool and then posting that tool somewhere for consumption.

Changing userspace code means changing userspace code, posting it
somewhere and you're done.

As Darren mentioned most companies are built around large organizations
with specific touch points.  The people who decide what the next features
will be for manageability aren't the same people who write the tools, who
aren't the same people that implement them in the BIOS and aren't the 
same people that would be submitting any of this into the kernel.  Asking
every new option to be whitelisted in the kernel and getting that translated
into upstream, into stable, into distros, updating tools for etc is a giant process
undertaking that companies will look at and say uh why?

I think Alan's suggestions are leading this the right way though, whitelisting
for certain unprivileged actions, blacklisting certain actions, looking for capabilities
but still letting tools with the right capabilities or permissions do what they
want to do.  It's a balancing act.

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

> I think I'd actually like to meld this with your other ideas and what I've 
> currently got.  What do you think of this approach:
> 
> 	/* kernel community doesn't feel userspace should have access at all
> 	  * or other kernel drivers use this
> 	  */
> 	if (blacklisted)
> 		return NO;
> 
> 	/* unprivileged access allowed */
>  	if (whitelisted & (capabilities && whitelist->capability_need) ==
>  	whitelist->capability_need))
>  		return ALLOWED;
>  
> 	/* not yet in whitelist, or need privs to do */
>  	if (capable(CAP_SYS_RAWIO))
>  		return ALLOWED;
>  
>  	return NO
> 

This looks sensible to me. Note that the middle case isn't necessarily
'unprviliged'. If the entyr is whitelisted and the capability_need is 0
then it means 'anyone' but you can also set any other appropriate
capability (eg CAP_NET_ADMIN for a WMI call that does stuff to the wifi).

Alan

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Alan Cox]

> I think I'd actually like to meld this with your other ideas and what I've 
> currently got.  What do you think of this approach:
> 
> 	/* kernel community doesn't feel userspace should have access at all
> 	  * or other kernel drivers use this
> 	  */
> 	if (blacklisted)
> 		return NO;
> 
> 	/* unprivileged access allowed */
>  	if (whitelisted & (capabilities && whitelist->capability_need) ==
>  	whitelist->capability_need))
>  		return ALLOWED;
>  
> 	/* not yet in whitelist, or need privs to do */
>  	if (capable(CAP_SYS_RAWIO))
>  		return ALLOWED;
>  
>  	return NO
> 

This looks sensible to me. Note that the middle case isn't necessarily
'unprviliged'. If the entyr is whitelisted and the capability_need is 0
then it means 'anyone' but you can also set any other appropriate
capability (eg CAP_NET_ADMIN for a WMI call that does stuff to the wifi).

Alan

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Darren Hart]

On Fri, Oct 13, 2017 at 08:46:11PM +0100, One Thousand Gnomes wrote:
> > I think I'd actually like to meld this with your other ideas and what I've 
> > currently got.  What do you think of this approach:
> > 
> > 	/* kernel community doesn't feel userspace should have access at all
> > 	  * or other kernel drivers use this
> > 	  */
> > 	if (blacklisted)
> > 		return NO;
> > 
> > 	/* unprivileged access allowed */
> >  	if (whitelisted & (capabilities && whitelist->capability_need) ==
> >  	whitelist->capability_need))
> >  		return ALLOWED;
> >  
> > 	/* not yet in whitelist, or need privs to do */
> >  	if (capable(CAP_SYS_RAWIO))
> >  		return ALLOWED;
> >  
> >  	return NO
> > 
> 
> This looks sensible to me. Note that the middle case isn't necessarily
> 'unprviliged'. If the entyr is whitelisted and the capability_need is 0
> then it means 'anyone' but you can also set any other appropriate
> capability (eg CAP_NET_ADMIN for a WMI call that does stuff to the wifi).

Thank you Alan. This model appears consistent in intent with some of the higher
level WMI access privileges in the Windows OS, and translate fairly well to
Linux.

This seems like a good model at least for the dell smbios driver in this thread.

I do have some concerns about the ability to audit the buffer in the general
case. Dell uses a sensible buffer format with 'command' and 'class' fields, but
the WMI spec explicitly makes no requirement on the format of the buffer. This
may prove much harder to implement for other vendors depending on their format -
but perhaps we accept that and deal with that as it comes up.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v7 10/15] platform/x86: dell-smbios: add filtering capability for requests

[Darren Hart]

On Fri, Oct 13, 2017 at 05:56:03PM +0200, Greg KH wrote:
> Just want to respond to this part first:
> 
> On Fri, Oct 13, 2017 at 03:03:10PM +0000, Mario.Limonciello@dell.com wrote:
> > Take off your "kernel" hat and put on a "customer" hat for a few moments
> > while I try to put this in practical terms why the whitelist approach doesn't
> > scale for what I'm trying to do.
> 
> Heh, you do know my background in running an enterprise kernel team,
> right? :)
> 
> > Let's say hypothetically a future version of this series that has only whitelisted
> > commands and tokens lands in a kernel that's in the next Ubuntu LTS, RHEL
> > release etc.
> > Hardware coming out about that time works fine, you can control the various
> > knobs.  Then later that year some new headless hardware is released that has
> > zigbee controllers that work with inbox kernel drivers but you can't turn them 
> > on and off any way BUT through the manageability interface (it's headless!).
> > In the manageability interface we also offer a new class/select or token that 
> > can control the GPIO that turns on/off these radios.
> 
> It's the "later" that you are missing here.  We only have code today for
> hardware we have today.  If you come out with new hardware, you need new
> kernel drivers for it, and as such, "old" enterprise kernels will just
> not work properly.
> 
> It's always been that way, this is nothing new, we can't predict the
> future, and is one big reason why I think the whole "enterprise" distro
> market is wrong and going to fail in the end :)

Just because it's always been that way, doesn't mean it has to continue
to be that way. We have a few examples where we support implementing
what was formerly kernel space in userspace, such as FUSE and libusb.
The details are different, but the idea is the same. Allow people to do
things they need to do without having to modify the kernel.

> 
> Same goes for that new device id for the wifi chip, or the video camera
> or the fingerprint reader.  Those have to be added to the kernel, and if
> the distro so desires, backported to their old and crufty version.
> 
> This has been happening for two decades now, somehow coming up with a
> "raw" pipe from userspace to the kernel to control the hardware just
> because you don't want to update the kernel code, isn't going to solve
> the issue here (hint, you now have to update your userspace code, why is
> that suddenly easier than the kernel?)

Even if updating the kernel was just as easy as updating a management
application they control, that's still twice the work.

> I know hardware companies want to stop writing software for their new
> hardware designs.  I too want a pony :)
> 
> Sorry, this argument isn't going to fly.

Well... I don't think reducing the amount of software required to
support a new platform is a bad thing.

I'm curious for your take on Alan's CAPs recommendation.

-- 
Darren Hart
VMware Open Source Technology Center