[PATCH v3 0/8] Introduce support for Dell SMBIOS over WMI

[PATCH v3 0/8] 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 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.

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 (8):
  platform/x86: wmi: Add new method wmidev_evaluate_method
  platform/x86: dell-smbios: Introduce a WMI-ACPI interface
  platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check
  platform/x86: wmi: create character devices when requested by drivers
  platform/x86: dell-wmi-smbios: introduce character device for
    userspace
  platform/x86: dell-wmi-smbios: Add a sysfs interface for SMBIOS tokens
  platform/x86: Kconfig: Change the default settings for dell-wmi-smbios
  platform/x86: dell-wmi-smbios: clean up wmi descriptor check

 Documentation/ABI/testing/dell-wmi-smbios          |  11 +
 .../ABI/testing/sysfs-platform-dell-wmi-smbios     |  16 ++
 MAINTAINERS                                        |   6 +
 drivers/platform/x86/Kconfig                       |  15 +-
 drivers/platform/x86/dell-smbios.c                 | 315 +++++++++++++++++++--
 drivers/platform/x86/dell-smbios.h                 |  15 +-
 drivers/platform/x86/dell-wmi.c                    |  75 +----
 drivers/platform/x86/wmi.c                         | 126 ++++++++-
 include/linux/wmi.h                                |   7 +
 include/uapi/linux/dell-wmi-smbios.h               |  30 ++
 10 files changed, 498 insertions(+), 118 deletions(-)
 create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
 create mode 100644 include/uapi/linux/dell-wmi-smbios.h

-- 
2.14.1

[PATCH v3 1/8] 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 v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Mario Limonciello]

The driver currently uses an SMI interface which grants direct access
to physical memory to the firmware SMM methods via a pointer.

Now add a WMI-ACPI interface that is detected by WMI probe and preferred
over the SMI interface.

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.

As a result, this change removes the dependency on this driver on the
dcdbas kernel module.  It's now an optional compilation option.

When modifying this, add myself to MAINTAINERS.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 MAINTAINERS                        |   6 ++
 drivers/platform/x86/Kconfig       |  14 ++--
 drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++-----
 drivers/platform/x86/dell-smbios.h |  15 ++++-
 4 files changed, 138 insertions(+), 25 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 08b96f77f618..6d76b09f46cc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3990,6 +3990,12 @@ S:	Maintained
 F:	drivers/hwmon/dell-smm-hwmon.c
 F:	include/uapi/linux/i8k.h
 
+DELL SMBIOS DRIVER
+M:	Pali Rohár <pali.rohar@gmail.com>
+M:	Mario Limonciello <mario.limonciello@dell.com>
+S:	Maintained
+F:	drivers/platform/x86/dell-smbios.*
+
 DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
 M:	Doug Warzecha <Douglas_Warzecha@dell.com>
 S:	Maintained
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 1f7959ff055c..415886d7a857 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -92,13 +92,17 @@ config ASUS_LAPTOP
 	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
 
 config DELL_SMBIOS
-	tristate
-	select DCDBAS
+	tristate "Dell SMBIOS calling interface"
+	depends on ACPI_WMI
 	---help---
-	This module provides common functions for kernel modules using
-	Dell SMBIOS.
+	This module provides common functions for kernel modules and
+        userspace using Dell SMBIOS.
+
+	If you have a Dell computer, say Y or M here.
 
-	If you have a Dell laptop, say Y or M here.
+	If you have a machine from < 2007 without a WMI interface you
+	may also want to enable CONFIG_DCDBAS to allow this driver to
+	work.
 
 config DELL_LAPTOP
 	tristate "Dell Laptop Extras"
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index e9b1ca07c872..4472817ee045 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -4,6 +4,7 @@
  *  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.
  *
  *  Based on documentation in the libsmbios package:
  *  Copyright (C) 2005-2014 Dell Inc.
@@ -22,9 +23,15 @@
 #include <linux/mutex.h>
 #include <linux/slab.h>
 #include <linux/io.h>
-#include "../../firmware/dcdbas.h"
+#include <linux/wmi.h>
 #include "dell-smbios.h"
 
+#ifdef CONFIG_DCDBAS
+#include "../../firmware/dcdbas.h"
+#endif
+
+#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+
 struct calling_interface_structure {
 	struct dmi_header header;
 	u16 cmdIOAddress;
@@ -33,12 +40,14 @@ struct calling_interface_structure {
 	struct calling_interface_token tokens[];
 } __packed;
 
-static struct calling_interface_buffer *buffer;
+static void *buffer;
+static size_t bufferlen;
 static DEFINE_MUTEX(buffer_mutex);
 
 static int da_command_address;
 static int da_command_code;
 static int da_num_tokens;
+struct wmi_device *wmi_dev;
 static struct calling_interface_token *da_tokens;
 
 int dell_smbios_error(int value)
@@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
 	dell_smbios_clear_buffer();
+	if (wmi_dev)
+		return &((struct wmi_calling_interface_buffer *) buffer)->smi;
 	return buffer;
 }
 EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
 
 void dell_smbios_clear_buffer(void)
 {
-	memset(buffer, 0, sizeof(struct calling_interface_buffer));
+	memset(buffer, 0, bufferlen);
 }
 EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
 
@@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void)
 }
 EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);
 
-void dell_smbios_send_request(int class, int select)
+static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
+{
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_buffer input;
+	union acpi_object *obj;
+	acpi_status status;
+
+	input.length = sizeof(struct wmi_calling_interface_buffer);
+	input.pointer = buf;
+
+	status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output);
+	if (ACPI_FAILURE(status)) {
+		pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
+			buf->smi.class, buf->smi.select,
+			buf->smi.input[0], buf->smi.input[1],
+			buf->smi.input[2], buf->smi.input[3]);
+			return -EIO;
+	}
+	obj = (union acpi_object *)output.pointer;
+	if (obj->type != ACPI_TYPE_BUFFER) {
+		pr_err("invalid type : %d\n", obj->type);
+		return -EIO;
+	}
+	memcpy(buf, obj->buffer.pointer, input.length);
+
+	return 0;
+}
+
+#ifdef CONFIG_DCDBAS
+static void run_smi_smbios_call(struct calling_interface_buffer *buf)
 {
 	struct smi_cmd command;
 
@@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select)
 	command.command_code = da_command_code;
 	command.ebx = virt_to_phys(buffer);
 	command.ecx = 0x42534931;
-
-	buffer->class = class;
-	buffer->select = select;
-
 	dcdbas_smi_request(&command);
 }
+#else
+static void run_smi_smbios_call(struct calling_interface_buffer *buf) {}
+#endif /* CONFIG_DCDBAS */
+
+void dell_smbios_send_request(int class, int select)
+{
+	if (wmi_dev) {
+		struct wmi_calling_interface_buffer *buf = buffer;
+
+		buf->smi.class = class;
+		buf->smi.select = select;
+		run_wmi_smbios_call(buf);
+	} else {
+		struct calling_interface_buffer *buf = buffer;
+
+		buf->class = class;
+		buf->select = select;
+		run_smi_smbios_call(buf);
+	}
+}
 EXPORT_SYMBOL_GPL(dell_smbios_send_request);
 
 struct calling_interface_token *dell_smbios_find_token(int tokenid)
@@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
-static int __init dell_smbios_init(void)
+static int dell_smbios_wmi_probe(struct wmi_device *wdev)
+{
+	/* no longer need the SMI page */
+	free_page((unsigned long)buffer);
+
+	/* WMI buffer should be 32k */
+	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+	if (!buffer)
+		return -ENOMEM;
+	bufferlen = sizeof(struct wmi_calling_interface_buffer);
+	wmi_dev = wdev;
+	return 0;
+}
+
+static int dell_smbios_wmi_remove(struct wmi_device *wdev)
 {
-	int ret;
+	wmi_dev = NULL;
+	free_pages((unsigned long)buffer, 3);
+	return 0;
+}
 
+static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
+	{ .guid_string = DELL_WMI_SMBIOS_GUID },
+	{ },
+};
+
+static struct wmi_driver dell_wmi_smbios_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 dell_smbios_init(void)
+{
 	dmi_walk(find_tokens, NULL);
 
 	if (!da_tokens)  {
@@ -181,34 +270,39 @@ static int __init dell_smbios_init(void)
 		return -ENODEV;
 	}
 
+#ifdef CONFIG_DCDBAS
 	/*
 	 * 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);
+	bufferlen = sizeof(struct calling_interface_buffer);
+#else
+	buffer = NULL;
+#endif /* CONFIG_DCDBAS */
+	wmi_driver_register(&dell_wmi_smbios_driver);
+
 	if (!buffer) {
-		ret = -ENOMEM;
-		goto fail_buffer;
+		kfree(da_tokens);
+		return -ENOMEM;
 	}
-
 	return 0;
-
-fail_buffer:
-	kfree(da_tokens);
-	return ret;
 }
 
 static void __exit dell_smbios_exit(void)
 {
 	kfree(da_tokens);
 	free_page((unsigned long)buffer);
+	wmi_driver_unregister(&dell_wmi_smbios_driver);
 }
 
 subsys_initcall(dell_smbios_init);
 module_exit(dell_smbios_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("Common functions for kernel modules using Dell SMBIOS");
 MODULE_LICENSE("GPL");
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 45cbc2292cd3..2f6fce81ee69 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -4,6 +4,7 @@
  *  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.
  *
  *  Based on documentation in the libsmbios package:
  *  Copyright (C) 2005-2014 Dell Inc.
@@ -18,9 +19,10 @@
 
 struct notifier_block;
 
-/* This structure will be modified by the firmware when we enter
- * system management mode, hence the volatiles */
-
+/* If called through fallback SMI rather than WMI 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;
@@ -28,6 +30,13 @@ struct calling_interface_buffer {
 	volatile u32 output[4];
 } __packed;
 
+struct wmi_calling_interface_buffer {
+	struct calling_interface_buffer smi;
+	u32 argattrib;
+	u32 blength;
+	u8 data[32724];
+} __packed;
+
 struct calling_interface_token {
 	u16 tokenID;
 	u16 location;
-- 
2.14.1

[PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Mario Limonciello]

The Dell WMI descriptor check is used as an indication that WMI
calls are safe to run both when used with the notification
ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.

As some code in dell-wmi-smbios is already a prerequisite for
dell-wmi, move the code for performing the descriptor check into
dell-wmi-smbios and let both drivers use it from there.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 drivers/platform/x86/dell-smbios.c | 81 ++++++++++++++++++++++++++++++++++++++
 drivers/platform/x86/dell-smbios.h |  3 ++
 drivers/platform/x86/dell-wmi.c    | 75 +----------------------------------
 3 files changed, 85 insertions(+), 74 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 4472817ee045..5d793b012e5e 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -31,6 +31,7 @@
 #endif
 
 #define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
+#define DELL_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
 
 struct calling_interface_structure {
 	struct dmi_header header;
@@ -226,8 +227,87 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+/*
+ * 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
+ */
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
+{
+	union acpi_object *obj = NULL;
+	struct wmi_device *desc_dev;
+	u32 *desc_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);
+		if (obj->buffer.length < 16) {
+			ret = -EINVAL;
+			goto out;
+		}
+	}
+	desc_buffer = (u32 *)obj->buffer.pointer;
+
+	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
+			8, desc_buffer);
+
+	if (desc_buffer[2] != 0 && desc_buffer[2] != 1)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
+			desc_buffer[2]);
+
+	if (desc_buffer[3] != 4096)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\n",
+			desc_buffer[3]);
+
+	*version = desc_buffer[2];
+	ret = 0;
+
+	dev_info(&wdev->dev, "Detected Dell WMI interface version %u\n",
+		*version);
+
+out:
+	kfree(obj);
+	put_device(&desc_dev->dev);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dell_wmi_check_descriptor_buffer);
+
+
 static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 {
+	int ret;
+	u32 interface_version;
+
+	ret = dell_wmi_check_descriptor_buffer(wdev, &interface_version);
+	if (ret)
+		return -ENODEV;
+
 	/* no longer need the SMI page */
 	free_page((unsigned long)buffer);
 
@@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 	if (!buffer)
 		return -ENOMEM;
 	bufferlen = sizeof(struct wmi_calling_interface_buffer);
+
 	wmi_dev = wdev;
 	return 0;
 }
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index 2f6fce81ee69..be9ec1ccf8bf 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -17,6 +17,8 @@
 #ifndef _DELL_SMBIOS_H_
 #define _DELL_SMBIOS_H_
 
+#include <linux/wmi.h>
+
 struct notifier_block;
 
 /* If called through fallback SMI rather than WMI this structure will be
@@ -62,5 +64,6 @@ enum dell_laptop_notifier_actions {
 int dell_laptop_register_notifier(struct notifier_block *nb);
 int dell_laptop_unregister_notifier(struct notifier_block *nb);
 void dell_laptop_call_notifier(unsigned long action, void *data);
+int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version);
 
 #endif
diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
index 1fbef560ca67..b3bb8cdd27bf 100644
--- a/drivers/platform/x86/dell-wmi.c
+++ b/drivers/platform/x86/dell-wmi.c
@@ -46,7 +46,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,78 +616,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
- */
-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);
-		if (obj->buffer.length < 16) {
-			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",
-			8, buffer);
-
-	if (buffer[2] != 0 && buffer[2] != 1)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%d)\n",
-			buffer[2]);
-
-	if (buffer[3] != 4096)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid buffer length (%d)\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:
  *
@@ -732,7 +659,7 @@ static int dell_wmi_probe(struct wmi_device *wdev)
 		return -ENOMEM;
 	dev_set_drvdata(&wdev->dev, priv);
 
-	err = dell_wmi_check_descriptor_buffer(wdev);
+	err = dell_wmi_check_descriptor_buffer(wdev, &priv->interface_version);
 	if (err)
 		return err;
 
-- 
2.14.1

[PATCH v3 4/8] platform/x86: wmi: create character devices when requested by 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 a set of functions in a
file_operations object the WMI bus driver will create a character
device that maps to those file operations.

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

This policy is selected as one driver may map and use multiple
GUIDs and it would be better to only expose a single character
device.

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>
---
 drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
 include/linux/wmi.h        |  1 +
 2 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 4d73a87c2ddf..17399df87948 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -34,7 +34,9 @@
 #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
 
 #include <linux/acpi.h>
+#include <linux/cdev.h>
 #include <linux/device.h>
+#include <linux/idr.h>
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho");
 MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
 MODULE_LICENSE("GPL");
 
+#define WMI_MAX_DEVS  MINORMASK
+static DEFINE_IDR(wmi_idr);
+static DEFINE_MUTEX(wmi_minor_lock);
 static LIST_HEAD(wmi_block_list);
 
 struct guid_block {
@@ -69,6 +74,8 @@ struct wmi_block {
 	struct wmi_device dev;
 	struct list_head list;
 	struct guid_block gblock;
+	struct cdev *cdev;
+	int minor;
 	struct acpi_device *acpi_device;
 	wmi_notify_handler handler;
 	void *handler_data;
@@ -86,6 +93,7 @@ struct wmi_block {
 #define ACPI_WMI_STRING      0x4	/* GUID takes & returns a string */
 #define ACPI_WMI_EVENT       0x8	/* GUID is an event */
 
+static dev_t wmi_devt;
 static bool debug_event;
 module_param(debug_event, bool, 0444);
 MODULE_PARM_DESC(debug_event,
@@ -782,21 +790,88 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
 	return 0;
 }
 
+static struct class wmi_bus_class = {
+	.name = "wmi_bus",
+};
+
+static int wmi_minor_get(struct wmi_block *wblock)
+{
+	int ret;
+
+	mutex_lock(&wmi_minor_lock);
+	ret = idr_alloc(&wmi_idr, wblock, 0, WMI_MAX_DEVS, GFP_KERNEL);
+	if (ret >= 0)
+		wblock->minor = ret;
+	else if (ret == -ENOSPC)
+		dev_err(&wblock->dev.dev, "too many wmi devices\n");
+
+	mutex_unlock(&wmi_minor_lock);
+	return ret;
+}
+
+static void wmi_minor_free(struct wmi_block *wblock)
+{
+	mutex_lock(&wmi_minor_lock);
+	idr_remove(&wmi_idr, wblock->minor);
+	mutex_unlock(&wmi_minor_lock);
+}
+
+
 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;
+	struct device *clsdev;
+	int ret = 0, devno;
 
 	if (ACPI_FAILURE(wmi_method_enable(wblock, 1)))
 		dev_warn(dev, "failed to enable device -- probing anyway\n");
 
+	/* driver wants a character device made */
+	if (wdriver->file_operations) {
+		dev->devt = wmi_devt;
+		wblock->cdev = cdev_alloc();
+		if (!wblock->cdev) {
+			dev_err(dev, "failed to allocate cdev\n");
+			return -ENOMEM;
+		}
+		cdev_init(wblock->cdev, wdriver->file_operations);
+		wblock->cdev->owner = wdriver->file_operations->owner;
+		ret = wmi_minor_get(wblock);
+		if (ret < 0)
+			return ret;
+		devno = MKDEV(MAJOR(wmi_devt), wblock->minor);
+		ret = cdev_add(wblock->cdev, devno, 1);
+		if (ret) {
+			dev_err(dev, "unable to create device %d:%d\n",
+			MAJOR(wmi_devt), wblock->minor);
+			goto err_probe_cdev;
+		}
+		clsdev = device_create(&wmi_bus_class, dev,
+				       MKDEV(MAJOR(wmi_devt), wblock->minor),
+				       NULL, "wmi/%s", wdriver->driver.name);
+
+		if (IS_ERR(clsdev)) {
+			dev_err(dev, "unable to create device %d:%d\n",
+			MAJOR(wmi_devt), wblock->minor);
+			ret = PTR_ERR(clsdev);
+			goto err_probe_class;
+		}
+	}
+
 	if (wdriver->probe) {
 		ret = wdriver->probe(dev_to_wdev(dev));
 		if (ret != 0 && ACPI_FAILURE(wmi_method_enable(wblock, 0)))
 			dev_warn(dev, "failed to disable device\n");
 	}
+	return ret;
+
+err_probe_class:
+	cdev_del(wblock->cdev);
+
+err_probe_cdev:
+	wmi_minor_free(wblock);
 
 	return ret;
 }
@@ -808,6 +883,13 @@ static int wmi_dev_remove(struct device *dev)
 		container_of(dev->driver, struct wmi_driver, driver);
 	int ret = 0;
 
+	if (wdriver->file_operations) {
+		device_destroy(&wmi_bus_class,
+			       MKDEV(MAJOR(wmi_devt), wblock->minor));
+		cdev_del(wblock->cdev);
+		wmi_minor_free(wblock);
+	}
+
 	if (wdriver->remove)
 		ret = wdriver->remove(dev_to_wdev(dev));
 
@@ -817,10 +899,6 @@ static int wmi_dev_remove(struct device *dev)
 	return ret;
 }
 
-static struct class wmi_bus_class = {
-	.name = "wmi_bus",
-};
-
 static struct bus_type wmi_bus_type = {
 	.name = "wmi",
 	.dev_groups = wmi_groups,
@@ -1270,8 +1348,17 @@ static int __init acpi_wmi_init(void)
 		goto err_unreg_bus;
 	}
 
+	error = alloc_chrdev_region(&wmi_devt, 0, WMI_MAX_DEVS, "wmi");
+	if (error < 0) {
+		pr_err("unable to allocate char dev region\n");
+		goto err_unreg_platform;
+	}
+
 	return 0;
 
+err_unreg_platform:
+	platform_driver_unregister(&acpi_wmi_driver);
+
 err_unreg_bus:
 	bus_unregister(&wmi_bus_type);
 
@@ -1283,6 +1370,7 @@ static int __init acpi_wmi_init(void)
 
 static void __exit acpi_wmi_exit(void)
 {
+	unregister_chrdev_region(wmi_devt, WMI_MAX_DEVS);
 	platform_driver_unregister(&acpi_wmi_driver);
 	bus_unregister(&wmi_bus_type);
 	class_unregister(&wmi_bus_class);
diff --git a/include/linux/wmi.h b/include/linux/wmi.h
index 2cd10c3b89e9..ba5f75a32f4c 100644
--- a/include/linux/wmi.h
+++ b/include/linux/wmi.h
@@ -47,6 +47,7 @@ struct wmi_device_id {
 struct wmi_driver {
 	struct device_driver driver;
 	const struct wmi_device_id *id_table;
+	const struct file_operations *file_operations;
 
 	int (*probe)(struct wmi_device *wdev);
 	int (*remove)(struct wmi_device *wdev);
-- 
2.14.1

[PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Mario Limonciello]

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

It provides an ioctl that will allow passing the 32k 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.

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.

The character device will only be created if the WMI interface was
found.

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

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

diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
new file mode 100644
index 000000000000..0a4200688cb0
--- /dev/null
+++ b/Documentation/ABI/testing/dell-wmi-smbios
@@ -0,0 +1,11 @@
+What:		/dev/wmi-dell-wmi-smbios
+Date:		October 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/dell-wmi-smbios.h>
+
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 5d793b012e5e..4174afbade13 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -24,6 +24,7 @@
 #include <linux/slab.h>
 #include <linux/io.h>
 #include <linux/wmi.h>
+#include <linux/uaccess.h>
 #include "dell-smbios.h"
 
 #ifdef CONFIG_DCDBAS
@@ -41,8 +42,9 @@ struct calling_interface_structure {
 	struct calling_interface_token tokens[];
 } __packed;
 
-static void *buffer;
-static size_t bufferlen;
+static void *internal_buffer;
+static size_t internal_bufferlen;
+static struct wmi_calling_interface_buffer *devfs_buffer;
 static DEFINE_MUTEX(buffer_mutex);
 
 static int da_command_address;
@@ -70,15 +72,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
 {
 	mutex_lock(&buffer_mutex);
 	dell_smbios_clear_buffer();
-	if (wmi_dev)
-		return &((struct wmi_calling_interface_buffer *) buffer)->smi;
-	return buffer;
+	if (!wmi_dev)
+		return internal_buffer;
+	return &((struct wmi_calling_interface_buffer *) internal_buffer)->smi;
 }
 EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
 
 void dell_smbios_clear_buffer(void)
 {
-	memset(buffer, 0, bufferlen);
+	memset(internal_buffer, 0, internal_bufferlen);
 }
 EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
 
@@ -135,13 +137,13 @@ static void run_smi_smbios_call(struct calling_interface_buffer *buf) {}
 void dell_smbios_send_request(int class, int select)
 {
 	if (wmi_dev) {
-		struct wmi_calling_interface_buffer *buf = buffer;
+		struct wmi_calling_interface_buffer *buf = internal_buffer;
 
 		buf->smi.class = class;
 		buf->smi.select = select;
 		run_wmi_smbios_call(buf);
 	} else {
-		struct calling_interface_buffer *buf = buffer;
+		struct calling_interface_buffer *buf = internal_buffer;
 
 		buf->class = class;
 		buf->select = select;
@@ -227,6 +229,49 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
+{
+	return nonseekable_open(inode, file);
+}
+
+static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
+	unsigned long arg)
+{
+	void __user *p = (void __user *) arg;
+	size_t size;
+	int ret = 0;
+
+	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
+		return -ENOTTY;
+
+	switch (cmd) {
+	case DELL_WMI_SMBIOS_CALL_CMD:
+		size = sizeof(struct wmi_calling_interface_buffer);
+		mutex_lock(&buffer_mutex);
+		if (copy_from_user(devfs_buffer, p, size)) {
+			ret = -EFAULT;
+			goto fail_smbios_cmd;
+		}
+		ret = run_wmi_smbios_call(devfs_buffer);
+		if (ret != 0)
+			goto fail_smbios_cmd;
+		if (copy_to_user(p, devfs_buffer, size))
+			ret = -EFAULT;
+fail_smbios_cmd:
+		mutex_unlock(&buffer_mutex);
+		break;
+	default:
+		pr_err("unsupported ioctl: %d.\n", cmd);
+		ret = -ENOIOCTLCMD;
+	}
+	return ret;
+}
+
 /*
  * Descriptor buffer is 128 byte long and contains:
  *
@@ -309,22 +354,33 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 		return -ENODEV;
 
 	/* no longer need the SMI page */
-	free_page((unsigned long)buffer);
+	free_page((unsigned long)internal_buffer);
 
 	/* WMI buffer should be 32k */
-	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
-	if (!buffer)
+	internal_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+	if (!internal_buffer)
 		return -ENOMEM;
-	bufferlen = sizeof(struct wmi_calling_interface_buffer);
+	internal_bufferlen = sizeof(struct wmi_calling_interface_buffer);
+
+	devfs_buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
+	if (!devfs_buffer) {
+		ret = -ENOMEM;
+		goto fail_devfs_buffer;
+	}
 
 	wmi_dev = wdev;
 	return 0;
+
+fail_devfs_buffer:
+	free_pages((unsigned long)internal_buffer, 3);
+	return ret;
 }
 
 static int dell_smbios_wmi_remove(struct wmi_device *wdev)
 {
 	wmi_dev = NULL;
-	free_pages((unsigned long)buffer, 3);
+	free_pages((unsigned long)internal_buffer, 3);
+	free_pages((unsigned long)devfs_buffer, 3);
 	return 0;
 }
 
@@ -333,6 +389,13 @@ static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
 	{ },
 };
 
+static const struct file_operations dell_wmi_smbios_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= dell_wmi_smbios_ioctl,
+	.open		= dell_wmi_smbios_open,
+	.release	= dell_wmi_smbios_release,
+};
+
 static struct wmi_driver dell_wmi_smbios_driver = {
 	.driver = {
 		.name = "dell-smbios",
@@ -340,6 +403,7 @@ static struct wmi_driver dell_wmi_smbios_driver = {
 	.probe = dell_smbios_wmi_probe,
 	.remove = dell_smbios_wmi_remove,
 	.id_table = dell_smbios_wmi_id_table,
+	.file_operations = &dell_wmi_smbios_fops,
 };
 
 static int __init dell_smbios_init(void)
@@ -356,14 +420,14 @@ static int __init dell_smbios_init(void)
 	 * 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);
-	bufferlen = sizeof(struct calling_interface_buffer);
+	internal_buffer = (void *)__get_free_page(GFP_KERNEL | GFP_DMA32);
+	internal_bufferlen = sizeof(struct calling_interface_buffer);
 #else
-	buffer = NULL;
+	internal_buffer = NULL;
 #endif /* CONFIG_DCDBAS */
 	wmi_driver_register(&dell_wmi_smbios_driver);
 
-	if (!buffer) {
+	if (!internal_buffer) {
 		kfree(da_tokens);
 		return -ENOMEM;
 	}
@@ -373,7 +437,7 @@ static int __init dell_smbios_init(void)
 static void __exit dell_smbios_exit(void)
 {
 	kfree(da_tokens);
-	free_page((unsigned long)buffer);
+	free_page((unsigned long)internal_buffer);
 	wmi_driver_unregister(&dell_wmi_smbios_driver);
 }
 
diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
index be9ec1ccf8bf..3b0e2e3f6ede 100644
--- a/drivers/platform/x86/dell-smbios.h
+++ b/drivers/platform/x86/dell-smbios.h
@@ -18,27 +18,10 @@
 #define _DELL_SMBIOS_H_
 
 #include <linux/wmi.h>
+#include <uapi/linux/dell-wmi-smbios.h>
 
 struct notifier_block;
 
-/* If called through fallback SMI rather than WMI 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 wmi_calling_interface_buffer {
-	struct calling_interface_buffer smi;
-	u32 argattrib;
-	u32 blength;
-	u8 data[32724];
-} __packed;
-
 struct calling_interface_token {
 	u16 tokenID;
 	u16 location;
diff --git a/include/uapi/linux/dell-wmi-smbios.h b/include/uapi/linux/dell-wmi-smbios.h
new file mode 100644
index 000000000000..adbe57dd055b
--- /dev/null
+++ b/include/uapi/linux/dell-wmi-smbios.h
@@ -0,0 +1,30 @@
+#ifndef _UAPI_DELL_WMI_SMBIOS_H_
+#define _UAPI_DELL_WMI_SMBIOS_H_
+
+#include <linux/ioctl.h>
+
+/* If called through fallback SMI rather than WMI 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 wmi_calling_interface_buffer {
+	struct calling_interface_buffer smi;
+	u32 argattrib;
+	u32 blength;
+	u8 data[32724];
+} __packed;
+
+#define DELL_WMI_SMBIOS_IOC			'D'
+/* run SMBIOS calling interface command
+ * note - 32k is too big for size, so this can not be encoded in macro properly
+ */
+#define DELL_WMI_SMBIOS_CALL_CMD	_IOWR(DELL_WMI_SMBIOS_IOC, 0, u8)
+
+#endif /* _UAPI_DELL_WMI_SMBIOS_H_ */
-- 
2.14.1

[PATCH v3 6/8] platform/x86: dell-wmi-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.

Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
---
 .../ABI/testing/sysfs-platform-dell-wmi-smbios     | 16 ++++++++++
 drivers/platform/x86/dell-smbios.c                 | 36 ++++++++++++++++++++++
 2 files changed, 52 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios

diff --git a/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios b/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
new file mode 100644
index 000000000000..4cbff5ffe380
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-dell-wmi-smbios
@@ -0,0 +1,16 @@
+What:		/sys/devices/platform/<platform>/tokens
+Date:		October 2017
+KernelVersion:	4.15
+Contact:	"Mario Limonciello" <mario.limonciello@dell.com>
+Description:
+		A read-only description of Dell platform tokens
+		available on the machine.
+
+		The tokens will be displayed in the following
+		machine readable format with each token on a
+		new line:
+
+		ID	Location	value
+
+		For example token:
+		5	5	3
diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index 4174afbade13..ac176953e46e 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -229,6 +229,34 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
 	}
 }
 
+static ssize_t tokens_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	size_t off = 0;
+	int i;
+
+	for (i = 0; i < da_num_tokens; i++) {
+		if (off > PAGE_SIZE)
+			break;
+		off += scnprintf(buf+off, PAGE_SIZE-off, "%04x\t%04x\t%04x\n",
+		da_tokens[i].tokenID, da_tokens[i].location,
+		da_tokens[i].value);
+	}
+
+	return off;
+}
+
+DEVICE_ATTR(tokens, 0400, tokens_show, NULL);
+
+static struct attribute *smbios_attrs[] = {
+	&dev_attr_tokens.attr,
+	NULL
+};
+
+static const struct attribute_group smbios_attribute_group = {
+	.attrs = smbios_attrs,
+};
+
 static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
 {
 	return nonseekable_open(inode, file);
@@ -367,10 +395,16 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
 		ret = -ENOMEM;
 		goto fail_devfs_buffer;
 	}
+	ret = sysfs_create_group(&wdev->dev.kobj, &smbios_attribute_group);
+	if (ret)
+		goto fail_create_group;
 
 	wmi_dev = wdev;
+	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
 	return 0;
 
+fail_create_group:
+	free_pages((unsigned long)devfs_buffer, 3);
 fail_devfs_buffer:
 	free_pages((unsigned long)internal_buffer, 3);
 	return ret;
@@ -381,6 +415,8 @@ static int dell_smbios_wmi_remove(struct wmi_device *wdev)
 	wmi_dev = NULL;
 	free_pages((unsigned long)internal_buffer, 3);
 	free_pages((unsigned long)devfs_buffer, 3);
+	sysfs_remove_group(&wdev->dev.kobj, &smbios_attribute_group);
+	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
 	return 0;
 }
 
-- 
2.14.1

[PATCH v3 7/8] platform/x86: Kconfig: Change the default settings for dell-wmi-smbios

[Mario Limonciello]

The dell-wmi-smbios driver should be enabled by default when ACPI_WMI
is enabled (like many other WMI drivers).

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

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 415886d7a857..a83c275019a4 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -94,6 +94,7 @@ config ASUS_LAPTOP
 config DELL_SMBIOS
 	tristate "Dell SMBIOS calling interface"
 	depends on ACPI_WMI
+	default ACPI_WMI
 	---help---
 	This module provides common functions for kernel modules and
         userspace using Dell SMBIOS.
-- 
2.14.1

[PATCH v3 8/8] platform/x86: dell-wmi-smbios: 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>
---
 drivers/platform/x86/dell-smbios.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
index ac176953e46e..d75b6971588e 100644
--- a/drivers/platform/x86/dell-smbios.c
+++ b/drivers/platform/x86/dell-smbios.c
@@ -346,16 +346,16 @@ int dell_wmi_check_descriptor_buffer(struct wmi_device *wdev, u32 *version)
 	}
 	desc_buffer = (u32 *)obj->buffer.pointer;
 
-	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
-		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
-			8, desc_buffer);
+	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)
+		dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
+			desc_buffer);
 
 	if (desc_buffer[2] != 0 && desc_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",
 			desc_buffer[2]);
 
 	if (desc_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",
 			desc_buffer[3]);
 
 	*version = desc_buffer[2];
-- 
2.14.1

Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 11394 bytes --]

On Thursday 28 September 2017 06:02:14 Mario Limonciello wrote:
> The driver currently uses an SMI interface which grants direct access
> to physical memory to the firmware SMM methods via a pointer.
> 
> Now add a WMI-ACPI interface that is detected by WMI probe and preferred
> over the SMI interface.
> 
> 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.
> 
> As a result, this change removes the dependency on this driver on the
> dcdbas kernel module.  It's now an optional compilation option.
> 
> When modifying this, add myself to MAINTAINERS.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  MAINTAINERS                        |   6 ++
>  drivers/platform/x86/Kconfig       |  14 ++--
>  drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++-----
>  drivers/platform/x86/dell-smbios.h |  15 ++++-
>  4 files changed, 138 insertions(+), 25 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 08b96f77f618..6d76b09f46cc 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3990,6 +3990,12 @@ S:	Maintained
>  F:	drivers/hwmon/dell-smm-hwmon.c
>  F:	include/uapi/linux/i8k.h
>  
> +DELL SMBIOS DRIVER
> +M:	Pali Rohár <pali.rohar@gmail.com>
> +M:	Mario Limonciello <mario.limonciello@dell.com>
> +S:	Maintained
> +F:	drivers/platform/x86/dell-smbios.*
> +
>  DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
>  M:	Doug Warzecha <Douglas_Warzecha@dell.com>
>  S:	Maintained
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 1f7959ff055c..415886d7a857 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -92,13 +92,17 @@ config ASUS_LAPTOP
>  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
>  
>  config DELL_SMBIOS
> -	tristate
> -	select DCDBAS
> +	tristate "Dell SMBIOS calling interface"
> +	depends on ACPI_WMI
>  	---help---
> -	This module provides common functions for kernel modules using
> -	Dell SMBIOS.
> +	This module provides common functions for kernel modules and
> +        userspace using Dell SMBIOS.
> +
> +	If you have a Dell computer, say Y or M here.
>  
> -	If you have a Dell laptop, say Y or M here.
> +	If you have a machine from < 2007 without a WMI interface you
> +	may also want to enable CONFIG_DCDBAS to allow this driver to
> +	work.
>  
>  config DELL_LAPTOP
>  	tristate "Dell Laptop Extras"
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index e9b1ca07c872..4472817ee045 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -4,6 +4,7 @@
>   *  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.
>   *
>   *  Based on documentation in the libsmbios package:
>   *  Copyright (C) 2005-2014 Dell Inc.
> @@ -22,9 +23,15 @@
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> -#include "../../firmware/dcdbas.h"
> +#include <linux/wmi.h>
>  #include "dell-smbios.h"
>  
> +#ifdef CONFIG_DCDBAS
> +#include "../../firmware/dcdbas.h"
> +#endif
> +
> +#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-B622A1EF5492"
> +
>  struct calling_interface_structure {
>  	struct dmi_header header;
>  	u16 cmdIOAddress;
> @@ -33,12 +40,14 @@ struct calling_interface_structure {
>  	struct calling_interface_token tokens[];
>  } __packed;
>  
> -static struct calling_interface_buffer *buffer;
> +static void *buffer;
> +static size_t bufferlen;
>  static DEFINE_MUTEX(buffer_mutex);
>  
>  static int da_command_address;
>  static int da_command_code;
>  static int da_num_tokens;
> +struct wmi_device *wmi_dev;
>  static struct calling_interface_token *da_tokens;
>  
>  int dell_smbios_error(int value)
> @@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
>  {
>  	mutex_lock(&buffer_mutex);
>  	dell_smbios_clear_buffer();
> +	if (wmi_dev)
> +		return &((struct wmi_calling_interface_buffer *) buffer)->smi;
>  	return buffer;
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
>  
>  void dell_smbios_clear_buffer(void)
>  {
> -	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> +	memset(buffer, 0, bufferlen);
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
>  
> @@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void)
>  }
>  EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);
>  
> -void dell_smbios_send_request(int class, int select)
> +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
> +{
> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	struct acpi_buffer input;
> +	union acpi_object *obj;
> +	acpi_status status;
> +
> +	input.length = sizeof(struct wmi_calling_interface_buffer);
> +	input.pointer = buf;
> +
> +	status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
> +			buf->smi.class, buf->smi.select,
> +			buf->smi.input[0], buf->smi.input[1],
> +			buf->smi.input[2], buf->smi.input[3]);
> +			return -EIO;
> +	}
> +	obj = (union acpi_object *)output.pointer;
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		pr_err("invalid type : %d\n", obj->type);
> +		return -EIO;
> +	}
> +	memcpy(buf, obj->buffer.pointer, input.length);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_DCDBAS
> +static void run_smi_smbios_call(struct calling_interface_buffer *buf)
>  {
>  	struct smi_cmd command;
>  
> @@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select)
>  	command.command_code = da_command_code;
>  	command.ebx = virt_to_phys(buffer);
>  	command.ecx = 0x42534931;
> -
> -	buffer->class = class;
> -	buffer->select = select;
> -
>  	dcdbas_smi_request(&command);
>  }
> +#else
> +static void run_smi_smbios_call(struct calling_interface_buffer *buf) {}
> +#endif /* CONFIG_DCDBAS */
> +
> +void dell_smbios_send_request(int class, int select)
> +{
> +	if (wmi_dev) {
> +		struct wmi_calling_interface_buffer *buf = buffer;
> +
> +		buf->smi.class = class;
> +		buf->smi.select = select;
> +		run_wmi_smbios_call(buf);
> +	} else {
> +		struct calling_interface_buffer *buf = buffer;
> +
> +		buf->class = class;
> +		buf->select = select;
> +		run_smi_smbios_call(buf);
> +	}
> +}
>  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
>  
>  struct calling_interface_token *dell_smbios_find_token(int tokenid)
> @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header *dm, void *dummy)
>  	}
>  }
>  
> -static int __init dell_smbios_init(void)
> +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> +{
> +	/* no longer need the SMI page */
> +	free_page((unsigned long)buffer);
> +
> +	/* WMI buffer should be 32k */
> +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> +	if (!buffer)
> +		return -ENOMEM;
> +	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> +	wmi_dev = wdev;
> +	return 0;
> +}
> +
> +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
>  {
> -	int ret;
> +	wmi_dev = NULL;
> +	free_pages((unsigned long)buffer, 3);
> +	return 0;
> +}

This code does not seem to be safe. dell_smbios_wmi_probe and
dell_smbios_wmi_remove are called at any time when kernel register new
device which matches some properties OR when user manually bind this
driver to that device.

buffer and wmi_dev is shared as a global variable which means that when
there are two devices which want to bind to this driver, kernel would
get double free at removing time.

Devices from the bus subsystem should not use global shared variables,
but rather allocate own memory...

Also note that user can at any time unbound device from driver and also
can bind it again.

> +static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
> +	{ .guid_string = DELL_WMI_SMBIOS_GUID },
> +	{ },
> +};
> +
> +static struct wmi_driver dell_wmi_smbios_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 dell_smbios_init(void)
> +{
>  	dmi_walk(find_tokens, NULL);
>  
>  	if (!da_tokens)  {
> @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void)
>  		return -ENODEV;
>  	}
>  
> +#ifdef CONFIG_DCDBAS
>  	/*
>  	 * 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);
> +	bufferlen = sizeof(struct calling_interface_buffer);
> +#else
> +	buffer = NULL;
> +#endif /* CONFIG_DCDBAS */
> +	wmi_driver_register(&dell_wmi_smbios_driver);
> +
>  	if (!buffer) {
> -		ret = -ENOMEM;
> -		goto fail_buffer;
> +		kfree(da_tokens);
> +		return -ENOMEM;
>  	}
> -
>  	return 0;
> -
> -fail_buffer:
> -	kfree(da_tokens);
> -	return ret;
>  }
>  
>  static void __exit dell_smbios_exit(void)
>  {
>  	kfree(da_tokens);
>  	free_page((unsigned long)buffer);
> +	wmi_driver_unregister(&dell_wmi_smbios_driver);
>  }
>  
>  subsys_initcall(dell_smbios_init);
>  module_exit(dell_smbios_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("Common functions for kernel modules using Dell SMBIOS");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
> index 45cbc2292cd3..2f6fce81ee69 100644
> --- a/drivers/platform/x86/dell-smbios.h
> +++ b/drivers/platform/x86/dell-smbios.h
> @@ -4,6 +4,7 @@
>   *  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.
>   *
>   *  Based on documentation in the libsmbios package:
>   *  Copyright (C) 2005-2014 Dell Inc.
> @@ -18,9 +19,10 @@
>  
>  struct notifier_block;
>  
> -/* This structure will be modified by the firmware when we enter
> - * system management mode, hence the volatiles */
> -
> +/* If called through fallback SMI rather than WMI 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;
> @@ -28,6 +30,13 @@ struct calling_interface_buffer {
>  	volatile u32 output[4];
>  } __packed;
>  
> +struct wmi_calling_interface_buffer {
> +	struct calling_interface_buffer smi;
> +	u32 argattrib;
> +	u32 blength;
> +	u8 data[32724];
> +} __packed;
> +
>  struct calling_interface_token {
>  	u16 tokenID;
>  	u16 location;
> 

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Thursday, September 28, 2017 2:54 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
> Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI
> interface
> 
> On Thursday 28 September 2017 06:02:14 Mario Limonciello wrote:
> > The driver currently uses an SMI interface which grants direct access
> > to physical memory to the firmware SMM methods via a pointer.
> >
> > Now add a WMI-ACPI interface that is detected by WMI probe and preferred
> > over the SMI interface.
> >
> > 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.
> >
> > As a result, this change removes the dependency on this driver on the
> > dcdbas kernel module.  It's now an optional compilation option.
> >
> > When modifying this, add myself to MAINTAINERS.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  MAINTAINERS                        |   6 ++
> >  drivers/platform/x86/Kconfig       |  14 ++--
> >  drivers/platform/x86/dell-smbios.c | 128 ++++++++++++++++++++++++++++++++-
> ----
> >  drivers/platform/x86/dell-smbios.h |  15 ++++-
> >  4 files changed, 138 insertions(+), 25 deletions(-)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 08b96f77f618..6d76b09f46cc 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3990,6 +3990,12 @@ S:	Maintained
> >  F:	drivers/hwmon/dell-smm-hwmon.c
> >  F:	include/uapi/linux/i8k.h
> >
> > +DELL SMBIOS DRIVER
> > +M:	Pali Rohár <pali.rohar@gmail.com>
> > +M:	Mario Limonciello <mario.limonciello@dell.com>
> > +S:	Maintained
> > +F:	drivers/platform/x86/dell-smbios.*
> > +
> >  DELL SYSTEMS MANAGEMENT BASE DRIVER (dcdbas)
> >  M:	Doug Warzecha <Douglas_Warzecha@dell.com>
> >  S:	Maintained
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 1f7959ff055c..415886d7a857 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -92,13 +92,17 @@ config ASUS_LAPTOP
> >  	  If you have an ACPI-compatible ASUS laptop, say Y or M here.
> >
> >  config DELL_SMBIOS
> > -	tristate
> > -	select DCDBAS
> > +	tristate "Dell SMBIOS calling interface"
> > +	depends on ACPI_WMI
> >  	---help---
> > -	This module provides common functions for kernel modules using
> > -	Dell SMBIOS.
> > +	This module provides common functions for kernel modules and
> > +        userspace using Dell SMBIOS.
> > +
> > +	If you have a Dell computer, say Y or M here.
> >
> > -	If you have a Dell laptop, say Y or M here.
> > +	If you have a machine from < 2007 without a WMI interface you
> > +	may also want to enable CONFIG_DCDBAS to allow this driver to
> > +	work.
> >
> >  config DELL_LAPTOP
> >  	tristate "Dell Laptop Extras"
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> > index e9b1ca07c872..4472817ee045 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -4,6 +4,7 @@
> >   *  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.
> >   *
> >   *  Based on documentation in the libsmbios package:
> >   *  Copyright (C) 2005-2014 Dell Inc.
> > @@ -22,9 +23,15 @@
> >  #include <linux/mutex.h>
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> > -#include "../../firmware/dcdbas.h"
> > +#include <linux/wmi.h>
> >  #include "dell-smbios.h"
> >
> > +#ifdef CONFIG_DCDBAS
> > +#include "../../firmware/dcdbas.h"
> > +#endif
> > +
> > +#define DELL_WMI_SMBIOS_GUID "A80593CE-A997-11DA-B012-
> B622A1EF5492"
> > +
> >  struct calling_interface_structure {
> >  	struct dmi_header header;
> >  	u16 cmdIOAddress;
> > @@ -33,12 +40,14 @@ struct calling_interface_structure {
> >  	struct calling_interface_token tokens[];
> >  } __packed;
> >
> > -static struct calling_interface_buffer *buffer;
> > +static void *buffer;
> > +static size_t bufferlen;
> >  static DEFINE_MUTEX(buffer_mutex);
> >
> >  static int da_command_address;
> >  static int da_command_code;
> >  static int da_num_tokens;
> > +struct wmi_device *wmi_dev;
> >  static struct calling_interface_token *da_tokens;
> >
> >  int dell_smbios_error(int value)
> > @@ -60,13 +69,15 @@ struct calling_interface_buffer
> *dell_smbios_get_buffer(void)
> >  {
> >  	mutex_lock(&buffer_mutex);
> >  	dell_smbios_clear_buffer();
> > +	if (wmi_dev)
> > +		return &((struct wmi_calling_interface_buffer *) buffer)->smi;
> >  	return buffer;
> >  }
> >  EXPORT_SYMBOL_GPL(dell_smbios_get_buffer);
> >
> >  void dell_smbios_clear_buffer(void)
> >  {
> > -	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> > +	memset(buffer, 0, bufferlen);
> >  }
> >  EXPORT_SYMBOL_GPL(dell_smbios_clear_buffer);
> >
> > @@ -76,7 +87,36 @@ void dell_smbios_release_buffer(void)
> >  }
> >  EXPORT_SYMBOL_GPL(dell_smbios_release_buffer);
> >
> > -void dell_smbios_send_request(int class, int select)
> > +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
> > +{
> > +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> > +	struct acpi_buffer input;
> > +	union acpi_object *obj;
> > +	acpi_status status;
> > +
> > +	input.length = sizeof(struct wmi_calling_interface_buffer);
> > +	input.pointer = buf;
> > +
> > +	status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output);
> > +	if (ACPI_FAILURE(status)) {
> > +		pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
> > +			buf->smi.class, buf->smi.select,
> > +			buf->smi.input[0], buf->smi.input[1],
> > +			buf->smi.input[2], buf->smi.input[3]);
> > +			return -EIO;
> > +	}
> > +	obj = (union acpi_object *)output.pointer;
> > +	if (obj->type != ACPI_TYPE_BUFFER) {
> > +		pr_err("invalid type : %d\n", obj->type);
> > +		return -EIO;
> > +	}
> > +	memcpy(buf, obj->buffer.pointer, input.length);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_DCDBAS
> > +static void run_smi_smbios_call(struct calling_interface_buffer *buf)
> >  {
> >  	struct smi_cmd command;
> >
> > @@ -85,12 +125,28 @@ void dell_smbios_send_request(int class, int select)
> >  	command.command_code = da_command_code;
> >  	command.ebx = virt_to_phys(buffer);
> >  	command.ecx = 0x42534931;
> > -
> > -	buffer->class = class;
> > -	buffer->select = select;
> > -
> >  	dcdbas_smi_request(&command);
> >  }
> > +#else
> > +static void run_smi_smbios_call(struct calling_interface_buffer *buf) {}
> > +#endif /* CONFIG_DCDBAS */
> > +
> > +void dell_smbios_send_request(int class, int select)
> > +{
> > +	if (wmi_dev) {
> > +		struct wmi_calling_interface_buffer *buf = buffer;
> > +
> > +		buf->smi.class = class;
> > +		buf->smi.select = select;
> > +		run_wmi_smbios_call(buf);
> > +	} else {
> > +		struct calling_interface_buffer *buf = buffer;
> > +
> > +		buf->class = class;
> > +		buf->select = select;
> > +		run_smi_smbios_call(buf);
> > +	}
> > +}
> >  EXPORT_SYMBOL_GPL(dell_smbios_send_request);
> >
> >  struct calling_interface_token *dell_smbios_find_token(int tokenid)
> > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header
> *dm, void *dummy)
> >  	}
> >  }
> >
> > -static int __init dell_smbios_init(void)
> > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > +{
> > +	/* no longer need the SMI page */
> > +	free_page((unsigned long)buffer);
> > +
> > +	/* WMI buffer should be 32k */
> > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> > +	if (!buffer)
> > +		return -ENOMEM;
> > +	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> > +	wmi_dev = wdev;
> > +	return 0;
> > +}
> > +
> > +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
> >  {
> > -	int ret;
> > +	wmi_dev = NULL;
> > +	free_pages((unsigned long)buffer, 3);
> > +	return 0;
> > +}
> 
> This code does not seem to be safe. dell_smbios_wmi_probe and
> dell_smbios_wmi_remove are called at any time when kernel register new
> device which matches some properties OR when user manually bind this
> driver to that device.
> 
I'll adjust for the assumptions I made about it only happening at module init.

> buffer and wmi_dev is shared as a global variable which means that when
> there are two devices which want to bind to this driver, kernel would
> get double free at removing time.

But there is only one GUID in id_table.  How can two devices bind to this?
This should be an impossible scenario.

> 
> Devices from the bus subsystem should not use global shared variables,
> but rather allocate own memory...

Part of the problem is that this module can operate in two modes now.
I think there will have to be global shared variables when operating in SMI
mode.  Or maybe put those bits into a platform_device for SMI mode usage?

The problem I think then becomes how do you handle dell_smbios_send_request?
Without global shared memory for the module the dell-laptop and dell-wmi
modules won't be able to use this.

> 
> Also note that user can at any time unbound device from driver and also
> can bind it again.
I'll make some adjustments to account for this.

> 
> > +static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
> > +	{ .guid_string = DELL_WMI_SMBIOS_GUID },
> > +	{ },
> > +};
> > +
> > +static struct wmi_driver dell_wmi_smbios_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 dell_smbios_init(void)
> > +{
> >  	dmi_walk(find_tokens, NULL);
> >
> >  	if (!da_tokens)  {
> > @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void)
> >  		return -ENODEV;
> >  	}
> >
> > +#ifdef CONFIG_DCDBAS
> >  	/*
> >  	 * 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);
> > +	bufferlen = sizeof(struct calling_interface_buffer);
> > +#else
> > +	buffer = NULL;
> > +#endif /* CONFIG_DCDBAS */
> > +	wmi_driver_register(&dell_wmi_smbios_driver);
> > +
> >  	if (!buffer) {
> > -		ret = -ENOMEM;
> > -		goto fail_buffer;
> > +		kfree(da_tokens);
> > +		return -ENOMEM;
> >  	}
> > -
> >  	return 0;
> > -
> > -fail_buffer:
> > -	kfree(da_tokens);
> > -	return ret;
> >  }
> >
> >  static void __exit dell_smbios_exit(void)
> >  {
> >  	kfree(da_tokens);
> >  	free_page((unsigned long)buffer);
> > +	wmi_driver_unregister(&dell_wmi_smbios_driver);
> >  }
> >
> >  subsys_initcall(dell_smbios_init);
> >  module_exit(dell_smbios_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("Common functions for kernel modules using Dell
> SMBIOS");
> >  MODULE_LICENSE("GPL");
> > diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-
> smbios.h
> > index 45cbc2292cd3..2f6fce81ee69 100644
> > --- a/drivers/platform/x86/dell-smbios.h
> > +++ b/drivers/platform/x86/dell-smbios.h
> > @@ -4,6 +4,7 @@
> >   *  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.
> >   *
> >   *  Based on documentation in the libsmbios package:
> >   *  Copyright (C) 2005-2014 Dell Inc.
> > @@ -18,9 +19,10 @@
> >
> >  struct notifier_block;
> >
> > -/* This structure will be modified by the firmware when we enter
> > - * system management mode, hence the volatiles */
> > -
> > +/* If called through fallback SMI rather than WMI 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;
> > @@ -28,6 +30,13 @@ struct calling_interface_buffer {
> >  	volatile u32 output[4];
> >  } __packed;
> >
> > +struct wmi_calling_interface_buffer {
> > +	struct calling_interface_buffer smi;
> > +	u32 argattrib;
> > +	u32 blength;
> > +	u8 data[32724];
> > +} __packed;
> > +
> >  struct calling_interface_token {
> >  	u16 tokenID;
> >  	u16 location;
> >
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Pali Rohár]

On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com wrote:
> > > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct dmi_header
> > *dm, void *dummy)
> > >  	}
> > >  }
> > >
> > > -static int __init dell_smbios_init(void)
> > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > > +{
> > > +	/* no longer need the SMI page */
> > > +	free_page((unsigned long)buffer);
> > > +
> > > +	/* WMI buffer should be 32k */
> > > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> > > +	if (!buffer)
> > > +		return -ENOMEM;
> > > +	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> > > +	wmi_dev = wdev;
> > > +	return 0;
> > > +}
> > > +
> > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
> > >  {
> > > -	int ret;
> > > +	wmi_dev = NULL;
> > > +	free_pages((unsigned long)buffer, 3);
> > > +	return 0;
> > > +}
> > 
> > This code does not seem to be safe. dell_smbios_wmi_probe and
> > dell_smbios_wmi_remove are called at any time when kernel register new
> > device which matches some properties OR when user manually bind this
> > driver to that device.
> > 
> I'll adjust for the assumptions I made about it only happening at module init.
> 
> > buffer and wmi_dev is shared as a global variable which means that when
> > there are two devices which want to bind to this driver, kernel would
> > get double free at removing time.
> 
> But there is only one GUID in id_table.

That is truth, but ...

> How can two devices bind to this?
> This should be an impossible scenario.

... in ACPI DSDT can be more WMI _WDG buffers which could lead to more
wmi buses and each could have own GUID. Therefore there is a theoretical
chance that specially prepared ACPI DSDT code can cause this problem.

> > Devices from the bus subsystem should not use global shared variables,
> > but rather allocate own memory...
> 
> Part of the problem is that this module can operate in two modes now.
> I think there will have to be global shared variables when operating in SMI
> mode.  Or maybe put those bits into a platform_device for SMI mode usage?
> 
> The problem I think then becomes how do you handle dell_smbios_send_request?
> Without global shared memory for the module the dell-laptop and dell-wmi
> modules won't be able to use this.

The whole problem is that SMBIOS calls are implemented via singleton
pattern. And this singleton "instance" needs to use something which is
not a singleton, but a dynamic objects (wmi device <--> driver pattern).

Idea how to handle it:
* put wmi smbios call function into own driver
* put smm smbios call function into own driver
* create dispatcher function which take first available device of one of
  the above driver and call on them smbios call function

This problem is very similar to problems in objects world... driver as a
class and device as a instance.

(Or if somebody has a better idea, let us know...)

> > Also note that user can at any time unbound device from driver and also
> > can bind it again.
> I'll make some adjustments to account for this.

To prevent crashing kernel, this needs to be written correctly. And user
should not be able to crash kernel just when trying to unbound driver
from the device.

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

Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Darren Hart]

On Wed, Sep 27, 2017 at 11:02:14PM -0500, Mario Limonciello wrote:
> The driver currently uses an SMI interface which grants direct access
> to physical memory to the firmware SMM methods via a pointer.
> 
> Now add a WMI-ACPI interface that is detected by WMI probe and preferred
> over the SMI interface.
> 
> 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.
> 
> As a result, this change removes the dependency on this driver on the
> dcdbas kernel module.  It's now an optional compilation option.
> 
> When modifying this, add myself to MAINTAINERS.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---

...

> +DELL SMBIOS DRIVER
> +M:	Pali Rohár <pali.rohar@gmail.com>
> +M:	Mario Limonciello <mario.limonciello@dell.com>
> +S:	Maintained
> +F:	drivers/platform/x86/dell-smbios.*

Pali, do you agree with this?

...

>  int dell_smbios_error(int value)
> @@ -60,13 +69,15 @@ struct calling_interface_buffer *dell_smbios_get_buffer(void)
>  {
>  	mutex_lock(&buffer_mutex);
>  	dell_smbios_clear_buffer();
> +	if (wmi_dev)
> +		return &((struct wmi_calling_interface_buffer *) buffer)->smi;
>  	return buffer;

Hrm, my hope had been to make this transparent at this level. ... This
may need more thought. I don't care for the casting here.... hopefully
enlightenment lies below...

> +static int run_wmi_smbios_call(struct wmi_calling_interface_buffer *buf)
> +{
> +	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
> +	struct acpi_buffer input;
> +	union acpi_object *obj;
> +	acpi_status status;
> +
> +	input.length = sizeof(struct wmi_calling_interface_buffer);
> +	input.pointer = buf;
> +
> +	status = wmidev_evaluate_method(wmi_dev, 0, 1, &input, &output);
> +	if (ACPI_FAILURE(status)) {
> +		pr_err("%x/%x [%x,%x,%x,%x] call failed\n",
> +			buf->smi.class, buf->smi.select,
> +			buf->smi.input[0], buf->smi.input[1],
> +			buf->smi.input[2], buf->smi.input[3]);
> +			return -EIO;
> +	}
> +	obj = (union acpi_object *)output.pointer;
> +	if (obj->type != ACPI_TYPE_BUFFER) {
> +		pr_err("invalid type : %d\n", obj->type);
> +		return -EIO;
> +	}

We ensure we don't write beyond buf, but we havne't ensured we don't read beyond
obj.


	if (obj->length != input.length) { // or maybe >= ??
		pr_err("invalid buffer length : %d\n", obj->length);
		return -EINVAL;
	}

> -static int __init dell_smbios_init(void)
> +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> +{
> +	/* no longer need the SMI page */
> +	free_page((unsigned long)buffer);
> +
> +	/* WMI buffer should be 32k */
> +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);

Assuming PAGE_SIZE here (I know, this driver, this architecture,
etc...). But, please use get_order() to determine number of pages
from a linear size:

__get_free_pages(GFP_KERNEL, get_order(32768));

> +	if (!buffer)
> +		return -ENOMEM;
> +	bufferlen = sizeof(struct wmi_calling_interface_buffer);

Use a consistent way to allocate and set the len. Maybe set bufferlen
above and use get_order(bufferlen).

> +	wmi_dev = wdev;
> +	return 0;
> +}
> +
> +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
>  {
> -	int ret;
> +	wmi_dev = NULL;
> +	free_pages((unsigned long)buffer, 3);
> +	return 0;
> +}
>  
> +static const struct wmi_device_id dell_smbios_wmi_id_table[] = {
> +	{ .guid_string = DELL_WMI_SMBIOS_GUID },
> +	{ },
> +};
> +
> +static struct wmi_driver dell_wmi_smbios_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 dell_smbios_init(void)
> +{
>  	dmi_walk(find_tokens, NULL);
>  
>  	if (!da_tokens)  {
> @@ -181,34 +270,39 @@ static int __init dell_smbios_init(void)
>  		return -ENODEV;
>  	}
>  
> +#ifdef CONFIG_DCDBAS

If this cannot be avoided, then use IS_ENABLED(CONFIG_DCDBAS).

I still think we should be to come up with a cleaner way to deal with
the two buffers than a bunch of #ifdefs and if (wdev) {} else {} blocks.

...
> diff --git a/drivers/platform/x86/dell-smbios.h b/drivers/platform/x86/dell-smbios.h
...
> -/* This structure will be modified by the firmware when we enter
> - * system management mode, hence the volatiles */
> -
> +/* If called through fallback SMI rather than WMI this structure will be
> + * modified by the firmware when we enter system management mode, hence the
> + * volatiles
> + */

Follow coding style when modifying comment blocks (even when they
didn't before) please. See coding-style 8) COmmenting.

/*
 * If called ...
 ...
 * volatiles.
 */

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Darren Hart]

On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote:
> The Dell WMI descriptor check is used as an indication that WMI
> calls are safe to run both when used with the notification
> ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.
> 
> As some code in dell-wmi-smbios is already a prerequisite for
> dell-wmi, move the code for performing the descriptor check into
> dell-wmi-smbios and let both drivers use it from there.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
...
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
...
>  
> +/*
> + * Descriptor buffer is 128 byte long and contains:
...
> +	if (obj->buffer.length != 128) {
> +		dev_err(&wdev->dev,
> +			"Dell descriptor buffer has invalid length (%d)\n",
> +			obj->buffer.length);

This seems odd. We call it an error (not a warning) if != 128, but we only abort
and return an error if it's < 16.

If it's an error, we should return an error code, if anything above 16 is
acceptable but 128 is preferred, the above should be a warning at best. (this
scenario seems unlikely).

> +		if (obj->buffer.length < 16) {
> +			ret = -EINVAL;
> +			goto out;
> +		}
> +	}
> +	desc_buffer = (u32 *)obj->buffer.pointer;
> +
> +	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)

This seems like it should be an || ?

(I see this is fixed in a later patch)

I would have suggested fixing it, then moving it - just from a pure ease of
review perspective. Perhaps not in the comment above that this is a verbatim
move, some fixes are coming in subsequent patches. This is worth noting for a
future develop who may be choosing what to backport.

>  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
>  {
> +	int ret;
> +	u32 interface_version;

Declarations in order of decreasing line length please.

> @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device *wdev)
>  	if (!buffer)
>  		return -ENOMEM;
>  	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> +

Stray whitespace, should have been in the previous patch I guess?


-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Darren Hart]

On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
> 
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call.  Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
> 
> When a WMI vendor driver declares a set of functions in a
> file_operations object the WMI bus driver will create a character
> device that maps to those file operations.
> 
> That character device will correspond to this path:
> /dev/wmi/$driver
> 
> This policy is selected as one driver may map and use multiple
> GUIDs and it would be better to only expose a single character
> device.
> 
> 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>
> ---
>  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/wmi.h        |  1 +
>  2 files changed, 94 insertions(+), 5 deletions(-)

+Greg, Rafael, Matthew, and Christoph

You each provided feedback regarding the method of exposing WMI methods
to userspace. This and subsequent patches from Mario lay some of the
core groundwork.

They implement an implicit whitelist as only drivers requesting the char
dev will see it created.

https://lkml.org/lkml/2017/9/28/8

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Darren Hart]

On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> This userspace character device will be used to perform SMBIOS calls
> from any applications.
> 
> It provides an ioctl that will allow passing the 32k 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.
> 
> 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.
> 
> The character device will only be created if the WMI interface was
> found.
> 
> The API for interacting with this interface is defined in documentation
> as well as a uapi header provides the format of the structures.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
>  Documentation/ABI/testing/dell-wmi-smbios |  11 ++++
>  drivers/platform/x86/dell-smbios.c        | 100 ++++++++++++++++++++++++------
>  drivers/platform/x86/dell-smbios.h        |  19 +-----
>  include/uapi/linux/dell-wmi-smbios.h      |  30 +++++++++
>  4 files changed, 124 insertions(+), 36 deletions(-)
>  create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
>  create mode 100644 include/uapi/linux/dell-wmi-smbios.h
> 
> diff --git a/Documentation/ABI/testing/dell-wmi-smbios b/Documentation/ABI/testing/dell-wmi-smbios
> new file mode 100644
> index 000000000000..0a4200688cb0
> --- /dev/null
> +++ b/Documentation/ABI/testing/dell-wmi-smbios
> @@ -0,0 +1,11 @@
> +What:		/dev/wmi-dell-wmi-smbios

/dev/wmi/dell-smbios

> +Date:		October 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/dell-wmi-smbios.h>
> +
> diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-smbios.c
> index 5d793b012e5e..4174afbade13 100644
> --- a/drivers/platform/x86/dell-smbios.c
> +++ b/drivers/platform/x86/dell-smbios.c
> @@ -24,6 +24,7 @@
>  #include <linux/slab.h>
>  #include <linux/io.h>
>  #include <linux/wmi.h>
> +#include <linux/uaccess.h>
>  #include "dell-smbios.h"
>  
>  #ifdef CONFIG_DCDBAS
> @@ -41,8 +42,9 @@ struct calling_interface_structure {
>  	struct calling_interface_token tokens[];
>  } __packed;
>  
> -static void *buffer;
> -static size_t bufferlen;
> +static void *internal_buffer;
> +static size_t internal_bufferlen;

A large portion of this changeset is dedicated to renaming these two variables,
but it isn't clear why, and not mentioned in the changelog. Seems like
unnecessary churn. Or if we really should rename it... can it be done earlier in
the series when we're working with those buffers for functional reasons?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 6/8] platform/x86: dell-wmi-smbios: Add a sysfs interface for SMBIOS tokens

[Darren Hart]

On Wed, Sep 27, 2017 at 11:02:18PM -0500, Mario Limonciello wrote:
> 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.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> ---
...
> +static ssize_t tokens_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	size_t off = 0;
> +	int i;
> +
> +	for (i = 0; i < da_num_tokens; i++) {
> +		if (off > PAGE_SIZE)
> +			break;
> +		off += scnprintf(buf+off, PAGE_SIZE-off, "%04x\t%04x\t%04x\n",

Minor Coding Style nit - spaces around binary operators: 3.1) Spaces

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 0/8] Introduce support for Dell SMBIOS over WMI

[Darren Hart]

On Wed, Sep 27, 2017 at 11:02:12PM -0500, Mario Limonciello wrote:
> 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 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.
> 

Mario,

Looking pretty good in general.

My biggest concern is around the dell-smbios.c "buffer" logic which is
starting to look really hacked together as it tries to deal with the
existing interface and the new WMI interface. Really seems like we
should be able to introduce one level of abstraction that would clean it
up. If not - can you explain your rationale in that patch?

My other concern is the freeform structure around creating the file
operations in each driver for the chardev IOCTL. It seems like we need
some kind of defined mapping from METHOD index to IOCTL number, or else
some way to advertise what it is?

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 963 bytes --]

On Saturday 30 September 2017 02:51:27 Darren Hart wrote:
> > +DELL SMBIOS DRIVER
> > +M:	Pali Rohár <pali.rohar@gmail.com>
> > +M:	Mario Limonciello <mario.limonciello@dell.com>
> > +S:	Maintained
> > +F:	drivers/platform/x86/dell-smbios.*
> 
> Pali, do you agree with this?

Yes, no problem.

> > -static int __init dell_smbios_init(void)
> > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > +{
> > +	/* no longer need the SMI page */
> > +	free_page((unsigned long)buffer);
> > +
> > +	/* WMI buffer should be 32k */
> > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> 
> Assuming PAGE_SIZE here (I know, this driver, this architecture,
> etc...). But, please use get_order() to determine number of pages
> from a linear size:
> 
> __get_free_pages(GFP_KERNEL, get_order(32768));

I agree that specifying size (instead of count) explicitly lead to more 
readable code.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg Kroah-Hartman]

On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> 
> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > For WMI operations that are only Set or Query read or write sysfs
> > attributes created by WMI vendor drivers make sense.
> > 
> > For other WMI operations that are run on Method, there needs to be a
> > way to guarantee to userspace that the results from the method call
> > belong to the data request to the method call.  Sysfs attributes don't
> > work well in this scenario because two userspace processes may be
> > competing at reading/writing an attribute and step on each other's
> > data.
> > 
> > When a WMI vendor driver declares a set of functions in a
> > file_operations object the WMI bus driver will create a character
> > device that maps to those file operations.
> > 
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> > 
> > This policy is selected as one driver may map and use multiple
> > GUIDs and it would be better to only expose a single character
> > device.
> > 
> > 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>
> > ---
> >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/wmi.h        |  1 +
> >  2 files changed, 94 insertions(+), 5 deletions(-)
> 
> +Greg, Rafael, Matthew, and Christoph
> 
> You each provided feedback regarding the method of exposing WMI methods
> to userspace. This and subsequent patches from Mario lay some of the
> core groundwork.
> 
> They implement an implicit whitelist as only drivers requesting the char
> dev will see it created.
> 
> https://lkml.org/lkml/2017/9/28/8

If you want patchs reviewed, it's best to actually cc: us on the patch
itself :(

RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Mario.Limonciello]

> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:gregkh@linuxfoundation.org]
> Sent: Saturday, September 30, 2017 3:12 AM
> To: Darren Hart <dvhart@infradead.org>
> Cc: 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; Rafael Wysocki
> <rjw@rjwysocki.net>; Matthew Garrett <mjg59@google.com>; Christoph Hellwig
> <hch@lst.de>
> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> >
> > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > For WMI operations that are only Set or Query read or write sysfs
> > > attributes created by WMI vendor drivers make sense.
> > >
> > > For other WMI operations that are run on Method, there needs to be a
> > > way to guarantee to userspace that the results from the method call
> > > belong to the data request to the method call.  Sysfs attributes don't
> > > work well in this scenario because two userspace processes may be
> > > competing at reading/writing an attribute and step on each other's
> > > data.
> > >
> > > When a WMI vendor driver declares a set of functions in a
> > > file_operations object the WMI bus driver will create a character
> > > device that maps to those file operations.
> > >
> > > That character device will correspond to this path:
> > > /dev/wmi/$driver
> > >
> > > This policy is selected as one driver may map and use multiple
> > > GUIDs and it would be better to only expose a single character
> > > device.
> > >
> > > 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>
> > > ---
> > >  drivers/platform/x86/wmi.c | 98
> +++++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/wmi.h        |  1 +
> > >  2 files changed, 94 insertions(+), 5 deletions(-)
> >
> > +Greg, Rafael, Matthew, and Christoph
> >
> > You each provided feedback regarding the method of exposing WMI methods
> > to userspace. This and subsequent patches from Mario lay some of the
> > core groundwork.
> >
> > They implement an implicit whitelist as only drivers requesting the char
> > dev will see it created.
> >
> > https://lkml.org/lkml/2017/9/28/8
> 
> If you want patchs reviewed, it's best to actually cc: us on the patch
> itself :(

Greg,

I'll make sure to CC you on V4 after I address the other concerns that have
been recently raised.

I think what's Darren's most interested in is the that conceptually this is
an approach you can agree with.

"A WMI vendor driver binds to the WMI bus and requests the WMI bus to create
a character device.  The WMI bus creates a character device /dev/wmi/$driver
which the WMI vendor driver will process all various file operations."

Thanks,

RE: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Friday, September 29, 2017 9:06 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: 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
> Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > This userspace character device will be used to perform SMBIOS calls
> > from any applications.
> >
> > It provides an ioctl that will allow passing the 32k 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.
> >
> > 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.
> >
> > The character device will only be created if the WMI interface was
> > found.
> >
> > The API for interacting with this interface is defined in documentation
> > as well as a uapi header provides the format of the structures.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> >  Documentation/ABI/testing/dell-wmi-smbios |  11 ++++
> >  drivers/platform/x86/dell-smbios.c        | 100 ++++++++++++++++++++++++---
> ---
> >  drivers/platform/x86/dell-smbios.h        |  19 +-----
> >  include/uapi/linux/dell-wmi-smbios.h      |  30 +++++++++
> >  4 files changed, 124 insertions(+), 36 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/dell-wmi-smbios
> >  create mode 100644 include/uapi/linux/dell-wmi-smbios.h
> >
> > diff --git a/Documentation/ABI/testing/dell-wmi-smbios
> b/Documentation/ABI/testing/dell-wmi-smbios
> > new file mode 100644
> > index 000000000000..0a4200688cb0
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/dell-wmi-smbios
> > @@ -0,0 +1,11 @@
> > +What:		/dev/wmi-dell-wmi-smbios
> 
> /dev/wmi/dell-smbios
> 
> > +Date:		October 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/dell-wmi-smbios.h>
> > +
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> > index 5d793b012e5e..4174afbade13 100644
> > --- a/drivers/platform/x86/dell-smbios.c
> > +++ b/drivers/platform/x86/dell-smbios.c
> > @@ -24,6 +24,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/io.h>
> >  #include <linux/wmi.h>
> > +#include <linux/uaccess.h>
> >  #include "dell-smbios.h"
> >
> >  #ifdef CONFIG_DCDBAS
> > @@ -41,8 +42,9 @@ struct calling_interface_structure {
> >  	struct calling_interface_token tokens[];
> >  } __packed;
> >
> > -static void *buffer;
> > -static size_t bufferlen;
> > +static void *internal_buffer;
> > +static size_t internal_bufferlen;
> 
> A large portion of this changeset is dedicated to renaming these two variables,
> but it isn't clear why, and not mentioned in the changelog. Seems like
> unnecessary churn. Or if we really should rename it... can it be done earlier in
> the series when we're working with those buffers for functional reasons?
> 
So the main reason for renaming was to make it easier to distinguish between
the character device buffer and internal buffer.  With my current (in process)
change it's much easier to distinguish.  I'll drop the renaming.

RE: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Friday, September 29, 2017 8:29 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: 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
> Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI
> descriptor check
> 
> On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote:
> > The Dell WMI descriptor check is used as an indication that WMI
> > calls are safe to run both when used with the notification
> > ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.
> >
> > As some code in dell-wmi-smbios is already a prerequisite for
> > dell-wmi, move the code for performing the descriptor check into
> > dell-wmi-smbios and let both drivers use it from there.
> >
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > ---
> ...
> > diff --git a/drivers/platform/x86/dell-smbios.c b/drivers/platform/x86/dell-
> smbios.c
> ...
> >
> > +/*
> > + * Descriptor buffer is 128 byte long and contains:
> ...
> > +	if (obj->buffer.length != 128) {
> > +		dev_err(&wdev->dev,
> > +			"Dell descriptor buffer has invalid length (%d)\n",
> > +			obj->buffer.length);
> 
> This seems odd. We call it an error (not a warning) if != 128, but we only abort
> and return an error if it's < 16.
> 
> If it's an error, we should return an error code, if anything above 16 is
> acceptable but 128 is preferred, the above should be a warning at best. (this
> scenario seems unlikely).

Hopefully the original author can speak up to the intentions here.  I would feel
that it should have errored out if it wasn't expected length too.

> 
> > +		if (obj->buffer.length < 16) {
> > +			ret = -EINVAL;
> > +			goto out;
> > +		}
> > +	}
> > +	desc_buffer = (u32 *)obj->buffer.pointer;
> > +
> > +	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
> 
> This seems like it should be an || ?
> 
> (I see this is fixed in a later patch)
> 
> I would have suggested fixing it, then moving it - just from a pure ease of
> review perspective. Perhaps not in the comment above that this is a verbatim
> move, some fixes are coming in subsequent patches. This is worth noting for a
> future develop who may be choosing what to backport.
> 

I'll re-order things so that the fixes come before the move.

> >  static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> >  {
> > +	int ret;
> > +	u32 interface_version;
> 
> Declarations in order of decreasing line length please.
> 
> > @@ -236,6 +316,7 @@ static int dell_smbios_wmi_probe(struct wmi_device
> *wdev)
> >  	if (!buffer)
> >  		return -ENOMEM;
> >  	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> > +
> 
> Stray whitespace, should have been in the previous patch I guess?
> 
> 
> --
> Darren Hart
> VMware Open Source Technology Center

RE: [PATCH v3 0/8] Introduce support for Dell SMBIOS over WMI

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Friday, September 29, 2017 9:17 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: 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
> Subject: Re: [PATCH v3 0/8] Introduce support for Dell SMBIOS over WMI
> 
> On Wed, Sep 27, 2017 at 11:02:12PM -0500, Mario Limonciello wrote:
> > 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 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.
> >
> 
> Mario,
> 
> Looking pretty good in general.
Darren,

Thanks.

> 
> My biggest concern is around the dell-smbios.c "buffer" logic which is
> starting to look really hacked together as it tries to deal with the
> existing interface and the new WMI interface. Really seems like we
> should be able to introduce one level of abstraction that would clean it
> up. If not - can you explain your rationale in that patch?
> 

I've got some changes that I'm working out issues with still that do it like
one of Pali's proposals.  It creates the WMI buffer associated to the WMI
device in a private memory structure.  Then the WMI remove function
will be responsible to clean it up.  So on unload no if/else magic.
During initialization I am reading a different flag from SMBIOS tables 
to tell if we'll support WMI or not even if the WMI device hasn't yet
been enumerated.  This approach will allow unbinding and rebinding
WMI without causing problems.

Does that sound better?  If not, can you please give me some more
direction on what you mean by one level of abstraction?  The "decision"
of which to handle will have to live somewhere so there's going to have
to be some if/else logic to know which way to go even if you abstract
further up.

> My other concern is the freeform structure around creating the file
> operations in each driver for the chardev IOCTL. It seems like we need
> some kind of defined mapping from METHOD index to IOCTL number, or else
> some way to advertise what it is?
> 

I was originally thinking it would be a good way to do this cleanly too, but my 
main concern is this one character device may handle multiple methods problem.
If you map method instance (index) to ioctl number you will most likely run into 
clashes.  So maybe it's worth not allowing that?

So I've got another idea.  How about instead of a variety of freeform ioctl per driver,
provide three ioctl functions for all the character devices that come in through
the WMI bus to use (say IOC 'W') with either read, write or write/read.  The WMI bus
should be able to know which driver to pass it on to by the character device used.

That example I mentioned the one character device for a driver with 3 different GUIDs
that could contain methods would require then splitting it up into say 3 different drivers
with 3 different character devices.

Otherwise this might still require some creative thinking.

RE: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Saturday, September 30, 2017 2:16 AM
> To: Darren Hart <dvhart@infradead.org>
> Cc: 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
> Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI
> interface
> 
> On Saturday 30 September 2017 02:51:27 Darren Hart wrote:
> > > +DELL SMBIOS DRIVER
> > > +M:	Pali Rohár <pali.rohar@gmail.com>
> > > +M:	Mario Limonciello <mario.limonciello@dell.com>
> > > +S:	Maintained
> > > +F:	drivers/platform/x86/dell-smbios.*
> >
> > Pali, do you agree with this?
> 
> Yes, no problem.
> 
> > > -static int __init dell_smbios_init(void)
> > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > > +{
> > > +	/* no longer need the SMI page */
> > > +	free_page((unsigned long)buffer);
> > > +
> > > +	/* WMI buffer should be 32k */
> > > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> >
> > Assuming PAGE_SIZE here (I know, this driver, this architecture,
> > etc...). But, please use get_order() to determine number of pages
> > from a linear size:
> >
> > __get_free_pages(GFP_KERNEL, get_order(32768));
> 
> I agree that specifying size (instead of count) explicitly lead to more
> readable code.
> 

Alright will adjust.

RE: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Friday, September 29, 2017 2:36 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> quasisec@google.com
> Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI
> interface
> 
> On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com wrote:
> > > > @@ -170,10 +226,43 @@ static void __init find_tokens(const struct
> dmi_header
> > > *dm, void *dummy)
> > > >  	}
> > > >  }
> > > >
> > > > -static int __init dell_smbios_init(void)
> > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > > > +{
> > > > +	/* no longer need the SMI page */
> > > > +	free_page((unsigned long)buffer);
> > > > +
> > > > +	/* WMI buffer should be 32k */
> > > > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> > > > +	if (!buffer)
> > > > +		return -ENOMEM;
> > > > +	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> > > > +	wmi_dev = wdev;
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
> > > >  {
> > > > -	int ret;
> > > > +	wmi_dev = NULL;
> > > > +	free_pages((unsigned long)buffer, 3);
> > > > +	return 0;
> > > > +}
> > >
> > > This code does not seem to be safe. dell_smbios_wmi_probe and
> > > dell_smbios_wmi_remove are called at any time when kernel register new
> > > device which matches some properties OR when user manually bind this
> > > driver to that device.
> > >
> > I'll adjust for the assumptions I made about it only happening at module init.
> >
> > > buffer and wmi_dev is shared as a global variable which means that when
> > > there are two devices which want to bind to this driver, kernel would
> > > get double free at removing time.
> >
> > But there is only one GUID in id_table.
> 
> That is truth, but ...
> 
> > How can two devices bind to this?
> > This should be an impossible scenario.
> 
> ... in ACPI DSDT can be more WMI _WDG buffers which could lead to more
> wmi buses and each could have own GUID. Therefore there is a theoretical
> chance that specially prepared ACPI DSDT code can cause this problem.

I guess I'm a bit confused - is this for protecting a user who patches their own
DSDT or from a vendor releasing a system with two _WDG buffers with the
same GUID?

I don't believe MOF has a concept of which WDG buffer GUIDs are assigned
to.  That could lead to massively undefined behavior too since the WDG buffer
will potentially assign different ASL to process for each GUID.
  
> 
> > > Devices from the bus subsystem should not use global shared variables,
> > > but rather allocate own memory...
> >
> > Part of the problem is that this module can operate in two modes now.
> > I think there will have to be global shared variables when operating in SMI
> > mode.  Or maybe put those bits into a platform_device for SMI mode usage?
> >
> > The problem I think then becomes how do you handle
> dell_smbios_send_request?
> > Without global shared memory for the module the dell-laptop and dell-wmi
> > modules won't be able to use this.
> 
> The whole problem is that SMBIOS calls are implemented via singleton
> pattern. And this singleton "instance" needs to use something which is
> not a singleton, but a dynamic objects (wmi device <--> driver pattern).
> 
> Idea how to handle it:
> * put wmi smbios call function into own driver
> * put smm smbios call function into own driver
> * create dispatcher function which take first available device of one of
>   the above driver and call on them smbios call function
> 
> This problem is very similar to problems in objects world... driver as a
> class and device as a instance.
> 
> (Or if somebody has a better idea, let us know...)

So I like the 3rd idea the most, and it's what I'm working on.  I've got some
problems with it that I'm still fixing, but unless someone tells me otherwise
I'll go for that with v4.

> 
> > > Also note that user can at any time unbound device from driver and also
> > > can bind it again.
> > I'll make some adjustments to account for this.
> 
> To prevent crashing kernel, this needs to be written correctly. And user
> should not be able to crash kernel just when trying to unbound driver
> from the device.
> 

Agree. 

Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1482 bytes --]

On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.com wrote:
> > > +/*
> > 
> > > + * Descriptor buffer is 128 byte long and contains:
> > ...
> > 
> > > +	if (obj->buffer.length != 128) {
> > > +		dev_err(&wdev->dev,
> > > +			"Dell descriptor buffer has invalid length (%d)\n",
> > > +			obj->buffer.length);
> > 
> > This seems odd. We call it an error (not a warning) if != 128, but
> > we only abort and return an error if it's < 16.
> > 
> > If it's an error, we should return an error code, if anything above
> > 16 is acceptable but 128 is preferred, the above should be a
> > warning at best. (this scenario seems unlikely).
> 
> Hopefully the original author can speak up to the intentions here.  I
> would feel that it should have errored out if it wasn't expected
> length too.

Code below access first 16 bytes of buffer. Therefore to prevent buffer 
overflow check for 16 bytes is needed.

But IIRC we decided to do not throw error and continue driver loading 
even when buffer length is not 128 (as expected by some Dell 
documentation) as it could be possible regression because driver itself 
does not depend on buffer length.

> > > +		if (obj->buffer.length < 16) {
> > > +			ret = -EINVAL;
> > > +			goto out;
> > > +		}
> > > +	}
> > > +	desc_buffer = (u32 *)obj->buffer.pointer;
> > > +
> > > +	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] !=
> > > 0x494D5720)

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a WMI-ACPI interface

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 5181 bytes --]

On Saturday 30 September 2017 22:01:04 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Friday, September 29, 2017 2:36 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org;
> > luto@kernel.org; quasisec@google.com
> > Subject: Re: [PATCH v3 2/8] platform/x86: dell-smbios: Introduce a
> > WMI-ACPI interface
> > 
> > On Thursday 28 September 2017 22:43:36 Mario.Limonciello@dell.com
> > wrote:
> > > > > @@ -170,10 +226,43 @@ static void __init find_tokens(const
> > > > > struct
> > 
> > dmi_header
> > 
> > > > *dm, void *dummy)
> > > > 
> > > > >  	}
> > > > >  
> > > > >  }
> > > > > 
> > > > > -static int __init dell_smbios_init(void)
> > > > > +static int dell_smbios_wmi_probe(struct wmi_device *wdev)
> > > > > +{
> > > > > +	/* no longer need the SMI page */
> > > > > +	free_page((unsigned long)buffer);
> > > > > +
> > > > > +	/* WMI buffer should be 32k */
> > > > > +	buffer = (void *)__get_free_pages(GFP_KERNEL, 3);
> > > > > +	if (!buffer)
> > > > > +		return -ENOMEM;
> > > > > +	bufferlen = sizeof(struct wmi_calling_interface_buffer);
> > > > > +	wmi_dev = wdev;
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int dell_smbios_wmi_remove(struct wmi_device *wdev)
> > > > > 
> > > > >  {
> > > > > 
> > > > > -	int ret;
> > > > > +	wmi_dev = NULL;
> > > > > +	free_pages((unsigned long)buffer, 3);
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > This code does not seem to be safe. dell_smbios_wmi_probe and
> > > > dell_smbios_wmi_remove are called at any time when kernel
> > > > register new device which matches some properties OR when user
> > > > manually bind this driver to that device.
> > > 
> > > I'll adjust for the assumptions I made about it only happening at
> > > module init.
> > > 
> > > > buffer and wmi_dev is shared as a global variable which means
> > > > that when there are two devices which want to bind to this
> > > > driver, kernel would get double free at removing time.
> > > 
> > > But there is only one GUID in id_table.
> > 
> > That is truth, but ...
> > 
> > > How can two devices bind to this?
> > > This should be an impossible scenario.
> > 
> > ... in ACPI DSDT can be more WMI _WDG buffers which could lead to
> > more wmi buses and each could have own GUID. Therefore there is a
> > theoretical chance that specially prepared ACPI DSDT code can
> > cause this problem.
> 
> I guess I'm a bit confused - is this for protecting a user who
> patches their own DSDT or from a vendor releasing a system with two
> _WDG buffers with the same GUID?

It is protection for memory corruption done by dell-smbios driver in 
case such DSDT would be parsed by kernel (either by user who patches 
original DSDT or when vendor created such DSDT by mistake or by 
malicious software which patch/provide such DSDT...).

DSDT is from kernel point of view external (possible untrusted) data. 
ACPI parser in kernel parse it if they are syntactically correct, but 
semantic or e.g. _WDG meaning is up to consumer -- in our case wmi and 
dell-smbios wmi drivers.

> I don't believe MOF has a concept of which WDG buffer GUIDs are
> assigned to.  That could lead to massively undefined behavior too
> since the WDG buffer will potentially assign different ASL to
> process for each GUID. 

Yes, that would lead to undefined behaviour. But still it should not 
crash kernel. Either treat such thing as "corrupted data" and refuse to 
use it or treat it in deterministic way, e.g. "first come, first serve" 
or load it driver for all guids...

E.g. DSDT on Thinkpads has more _WDG buffers and I was told that some 
machines with Nvidia graphics cards really have duplicate WMI GUIDs in 
_WDG. So such situation really happen in world, it is not just 
theoretical.

> > The whole problem is that SMBIOS calls are implemented via
> > singleton pattern. And this singleton "instance" needs to use
> > something which is not a singleton, but a dynamic objects (wmi
> > device <--> driver pattern).
> > 
> > Idea how to handle it:
> > * put wmi smbios call function into own driver
> > * put smm smbios call function into own driver
> > * create dispatcher function which take first available device of
> > one of
> > 
> >   the above driver and call on them smbios call function
> > 
> > This problem is very similar to problems in objects world... driver
> > as a class and device as a instance.
> > 
> > (Or if somebody has a better idea, let us know...)
> 
> So I like the 3rd idea the most, and it's what I'm working on.  I've
> got some problems with it that I'm still fixing, but unless someone
> tells me otherwise I'll go for that with v4.

All above 3 points (*) were meant as one solution. Putting wmi and smm 
calls into own drivers and then creating dispatcher function which would 
use available device of one of those two drivers.

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Andy Shevchenko]

On Sat, Sep 30, 2017 at 4:29 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, Sep 27, 2017 at 11:02:15PM -0500, Mario Limonciello wrote:
>> The Dell WMI descriptor check is used as an indication that WMI
>> calls are safe to run both when used with the notification
>> ASL/GUID pair as well as the SMBIOS calling ASL/GUID pair.
>>
>> As some code in dell-wmi-smbios is already a prerequisite for
>> dell-wmi, move the code for performing the descriptor check into
>> dell-wmi-smbios and let both drivers use it from there.

>> +     if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
>
> This seems like it should be an || ?
>
> (I see this is fixed in a later patch)

Interesting, I proposed to use strncmp(), but it's gone again. Perhaps
I missed what is downside of that.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 6/8] platform/x86: dell-wmi-smbios: Add a sysfs interface for SMBIOS tokens

[Andy Shevchenko]

On Sat, Sep 30, 2017 at 5:10 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Wed, Sep 27, 2017 at 11:02:18PM -0500, Mario Limonciello wrote:
>> 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.
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
>> ---
> ...
>> +static ssize_t tokens_show(struct device *dev,
>> +                        struct device_attribute *attr, char *buf)
>> +{
>> +     size_t off = 0;
>> +     int i;
>> +
>> +     for (i = 0; i < da_num_tokens; i++) {
>> +             if (off > PAGE_SIZE)
>> +                     break;
>> +             off += scnprintf(buf+off, PAGE_SIZE-off, "%04x\t%04x\t%04x\n",
>
> Minor Coding Style nit - spaces around binary operators: 3.1) Spaces

Fresh looking to this excerpt gives me an idea that we may actually
predict how many tokens to write beforehand. It makes output also
cleaner in a sense to not disrupt a last token in the middle.

Something like
tokens_to_print = min(da_num_tokens, (PAGE_SIZE  - 1) / 15);
?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg KH]

On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote:
> I think what's Darren's most interested in is the that conceptually this is
> an approach you can agree with.
> 
> "A WMI vendor driver binds to the WMI bus and requests the WMI bus to create
> a character device.  The WMI bus creates a character device /dev/wmi/$driver
> which the WMI vendor driver will process all various file operations."

We've been over this before, let's see that actual implementation which
defines the "will process all..." implementation to see if that is
actually an acceptable thing.

thanks,

greg k-h

RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Sunday, October 1, 2017 8:24 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: 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 v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote:
> > I think what's Darren's most interested in is the that conceptually this is
> > an approach you can agree with.
> >
> > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to
> create
> > a character device.  The WMI bus creates a character device /dev/wmi/$driver
> > which the WMI vendor driver will process all various file operations."
> 
> We've been over this before, let's see that actual implementation which
> defines the "will process all..." implementation to see if that is
> actually an acceptable thing.
> 
> thanks,
> 
> greg k-h

I'm working on changes still (that you'll be CC on), but I don't expect this specific 
concept to change in the future changes:

Bus creating character device:
https://patchwork.kernel.org/patch/9975283/

Driver requesting character device:
https://patchwork.kernel.org/patch/9975263/

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg KH]

On Sun, Oct 01, 2017 at 02:25:00PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Sunday, October 1, 2017 8:24 AM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: 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 v3 4/8] platform/x86: wmi: create character devices when
> > requested by drivers
> > 
> > On Sat, Sep 30, 2017 at 07:26:57PM +0000, Mario.Limonciello@dell.com wrote:
> > > I think what's Darren's most interested in is the that conceptually this is
> > > an approach you can agree with.
> > >
> > > "A WMI vendor driver binds to the WMI bus and requests the WMI bus to
> > create
> > > a character device.  The WMI bus creates a character device /dev/wmi/$driver
> > > which the WMI vendor driver will process all various file operations."
> > 
> > We've been over this before, let's see that actual implementation which
> > defines the "will process all..." implementation to see if that is
> > actually an acceptable thing.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I'm working on changes still (that you'll be CC on), but I don't expect this specific 
> concept to change in the future changes:
> 
> Bus creating character device:
> https://patchwork.kernel.org/patch/9975283/

A bit hard to comment on web pages, and I would like to point out your
obvious memory leak, but it's hard to do that this way :(

> Driver requesting character device:
> https://patchwork.kernel.org/patch/9975263/

There's other issues with this patch as well, I'll wait for a real one
in my inbox to be able to actually review it :(

thanks,

greg k-h

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Darren Hart]

On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> > 
> > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > For WMI operations that are only Set or Query read or write sysfs
> > > attributes created by WMI vendor drivers make sense.
> > > 
> > > For other WMI operations that are run on Method, there needs to be a
> > > way to guarantee to userspace that the results from the method call
> > > belong to the data request to the method call.  Sysfs attributes don't
> > > work well in this scenario because two userspace processes may be
> > > competing at reading/writing an attribute and step on each other's
> > > data.
> > > 
> > > When a WMI vendor driver declares a set of functions in a
> > > file_operations object the WMI bus driver will create a character
> > > device that maps to those file operations.
> > > 
> > > That character device will correspond to this path:
> > > /dev/wmi/$driver
> > > 
> > > This policy is selected as one driver may map and use multiple
> > > GUIDs and it would be better to only expose a single character
> > > device.
> > > 
> > > 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>
> > > ---
> > >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> > >  include/linux/wmi.h        |  1 +
> > >  2 files changed, 94 insertions(+), 5 deletions(-)
> > 
> > +Greg, Rafael, Matthew, and Christoph
> > 
> > You each provided feedback regarding the method of exposing WMI methods
> > to userspace. This and subsequent patches from Mario lay some of the
> > core groundwork.
> > 
> > They implement an implicit whitelist as only drivers requesting the char
> > dev will see it created.
> > 
> > https://lkml.org/lkml/2017/9/28/8
> 
> If you want patchs reviewed, it's best to actually cc: us on the patch
> itself :(
> 

Of course. I didn't send the series, but thought you should see it. I
could have asked Mario to resend, but I thought a pointer would have
made it easy enough to find in your lkml folder, and it would avoid
splitting the conversation which resending would inevitably lead to. I
pruned this one because Christoph gets upset if I don't.

We can wait for v4 I guess. And next time I want to get your take on
something someone doesn't Cc you on, I'll just ask them to resend the
whole series with you on Cc.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg Kroah-Hartman]

On Sun, Oct 01, 2017 at 05:57:20PM -0700, Darren Hart wrote:
> On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> > > 
> > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > > For WMI operations that are only Set or Query read or write sysfs
> > > > attributes created by WMI vendor drivers make sense.
> > > > 
> > > > For other WMI operations that are run on Method, there needs to be a
> > > > way to guarantee to userspace that the results from the method call
> > > > belong to the data request to the method call.  Sysfs attributes don't
> > > > work well in this scenario because two userspace processes may be
> > > > competing at reading/writing an attribute and step on each other's
> > > > data.
> > > > 
> > > > When a WMI vendor driver declares a set of functions in a
> > > > file_operations object the WMI bus driver will create a character
> > > > device that maps to those file operations.
> > > > 
> > > > That character device will correspond to this path:
> > > > /dev/wmi/$driver
> > > > 
> > > > This policy is selected as one driver may map and use multiple
> > > > GUIDs and it would be better to only expose a single character
> > > > device.
> > > > 
> > > > 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>
> > > > ---
> > > >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> > > >  include/linux/wmi.h        |  1 +
> > > >  2 files changed, 94 insertions(+), 5 deletions(-)
> > > 
> > > +Greg, Rafael, Matthew, and Christoph
> > > 
> > > You each provided feedback regarding the method of exposing WMI methods
> > > to userspace. This and subsequent patches from Mario lay some of the
> > > core groundwork.
> > > 
> > > They implement an implicit whitelist as only drivers requesting the char
> > > dev will see it created.
> > > 
> > > https://lkml.org/lkml/2017/9/28/8
> > 
> > If you want patchs reviewed, it's best to actually cc: us on the patch
> > itself :(
> > 
> 
> Of course. I didn't send the series, but thought you should see it. I
> could have asked Mario to resend, but I thought a pointer would have
> made it easy enough to find in your lkml folder, and it would avoid
> splitting the conversation which resending would inevitably lead to. I
> pruned this one because Christoph gets upset if I don't.
> 
> We can wait for v4 I guess. And next time I want to get your take on
> something someone doesn't Cc you on, I'll just ask them to resend the
> whole series with you on Cc.

Dude, that's the way it's always been, you know this!  Nothing new here,
respining a patch series is normal, and asking people to review patch
sets that you know already needs to be changed is strange.  Just do the
fixes, and resend it, that's the simplest, quickest, and easiest thing
to do for everyone involved.

Asking someone to review a patch based on a url just doesn't work, as
how can I point out where the memory leak is exactly?  :)

Reviewers get thousands of emails a week (or a day in some cases), and
making it as easy as possible for them to review the code is the key
here.

thanks,

greg k-h

Re: [PATCH v3 8/8] platform/x86: dell-wmi-smbios: clean up wmi descriptor check

[Andy Shevchenko]

On Thu, Sep 28, 2017 at 7:02 AM, Mario Limonciello
<mario.limonciello@dell.com> wrote:
> Some cases the wrong type was used for errors and checks can be
> done more cleanly.

Oops, I forgot about this patch, so, please, disregard my comment WRT
to strncmp() use to the other patch.


> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Reviewed-by: Edward O'Callaghan <quasisec@google.com>

Btw, missed Suggested-by?


> -       if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
> -               dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%*ph)\n",
> -                       8, desc_buffer);
> +       if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)
> +               dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
> +                       desc_buffer);

And as Darren pointed out, this fixes the logic bug as well.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 8/8] platform/x86: dell-wmi-smbios: clean up wmi descriptor check

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Monday, October 2, 2017 8:16 AM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; LKML <linux-kernel@vger.kernel.org>; Platform Driver
> <platform-driver-x86@vger.kernel.org>; Andy Lutomirski <luto@kernel.org>;
> quasisec@google.com; Pali Rohár <pali.rohar@gmail.com>
> Subject: Re: [PATCH v3 8/8] platform/x86: dell-wmi-smbios: clean up wmi
> descriptor check
> 
> On Thu, Sep 28, 2017 at 7:02 AM, Mario Limonciello
> <mario.limonciello@dell.com> wrote:
> > Some cases the wrong type was used for errors and checks can be
> > done more cleanly.
> 
> Oops, I forgot about this patch, so, please, disregard my comment WRT
> to strncmp() use to the other patch.
> 
> 
> > Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> > Reviewed-by: Edward O'Callaghan <quasisec@google.com>
> 
> Btw, missed Suggested-by?

Yes sorry about that.  I'll add that for when I get v4 out.

> 
> 
> > -       if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] != 0x494D5720)
> > -               dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature
> (%*ph)\n",
> > -                       8, desc_buffer);
> > +       if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0)
> > +               dev_warn(&wdev->dev, "Dell descriptor buffer has invalid signature
> (%8ph)\n",
> > +                       desc_buffer);
> 
> And as Darren pointed out, this fixes the logic bug as well.
> 
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Mario.Limonciello]

> -----Original Message-----
> From: Pali Rohár [mailto:pali.rohar@gmail.com]
> Sent: Saturday, September 30, 2017 3:01 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> quasisec@google.com
> Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI
> descriptor check
> 
> On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.com wrote:
> > > > +/*
> > >
> > > > + * Descriptor buffer is 128 byte long and contains:
> > > ...
> > >
> > > > +	if (obj->buffer.length != 128) {
> > > > +		dev_err(&wdev->dev,
> > > > +			"Dell descriptor buffer has invalid length (%d)\n",
> > > > +			obj->buffer.length);
> > >
> > > This seems odd. We call it an error (not a warning) if != 128, but
> > > we only abort and return an error if it's < 16.
> > >
> > > If it's an error, we should return an error code, if anything above
> > > 16 is acceptable but 128 is preferred, the above should be a
> > > warning at best. (this scenario seems unlikely).
> >
> > Hopefully the original author can speak up to the intentions here.  I
> > would feel that it should have errored out if it wasn't expected
> > length too.
> 
> Code below access first 16 bytes of buffer. Therefore to prevent buffer
> overflow check for 16 bytes is needed.
> 
> But IIRC we decided to do not throw error and continue driver loading
> even when buffer length is not 128 (as expected by some Dell
> documentation) as it could be possible regression because driver itself
> does not depend on buffer length.
> 

So I'm intending to change this in my next patch series.  I feel it should throw an
error when the buffer length isn't 128.

My logic is that if you don't see the proper buffer size (or the proper header)
then how can you trust that the rest of the data is reliable?  This means the format
has changed or this isn't a real descriptor as expected by Dell (say some other vendor
that has cloned the GUID).  It's better to abort in this situation.

> > > > +		if (obj->buffer.length < 16) {
> > > > +			ret = -EINVAL;
> > > > +			goto out;
> > > > +		}
> > > > +	}
> > > > +	desc_buffer = (u32 *)obj->buffer.pointer;
> > > > +
> > > > +	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] !=
> > > > 0x494D5720)
> 
> --
> Pali Rohár
> pali.rohar@gmail.com

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Darren Hart]

On Mon, Oct 02, 2017 at 11:24:53AM +0200, Greg Kroah-Hartman wrote:
> On Sun, Oct 01, 2017 at 05:57:20PM -0700, Darren Hart wrote:
> > On Sat, Sep 30, 2017 at 10:12:05AM +0200, Greg Kroah-Hartman wrote:
> > > On Fri, Sep 29, 2017 at 06:52:28PM -0700, Darren Hart wrote:
> > > > 
> > > > On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > > > > For WMI operations that are only Set or Query read or write sysfs
> > > > > attributes created by WMI vendor drivers make sense.
> > > > > 
> > > > > For other WMI operations that are run on Method, there needs to be a
> > > > > way to guarantee to userspace that the results from the method call
> > > > > belong to the data request to the method call.  Sysfs attributes don't
> > > > > work well in this scenario because two userspace processes may be
> > > > > competing at reading/writing an attribute and step on each other's
> > > > > data.
> > > > > 
> > > > > When a WMI vendor driver declares a set of functions in a
> > > > > file_operations object the WMI bus driver will create a character
> > > > > device that maps to those file operations.
> > > > > 
> > > > > That character device will correspond to this path:
> > > > > /dev/wmi/$driver
> > > > > 
> > > > > This policy is selected as one driver may map and use multiple
> > > > > GUIDs and it would be better to only expose a single character
> > > > > device.
> > > > > 
> > > > > 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>
> > > > > ---
> > > > >  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
> > > > >  include/linux/wmi.h        |  1 +
> > > > >  2 files changed, 94 insertions(+), 5 deletions(-)
> > > > 
> > > > +Greg, Rafael, Matthew, and Christoph
> > > > 
> > > > You each provided feedback regarding the method of exposing WMI methods
> > > > to userspace. This and subsequent patches from Mario lay some of the
> > > > core groundwork.
> > > > 
> > > > They implement an implicit whitelist as only drivers requesting the char
> > > > dev will see it created.
> > > > 
> > > > https://lkml.org/lkml/2017/9/28/8
> > > 
> > > If you want patchs reviewed, it's best to actually cc: us on the patch
> > > itself :(
> > > 
> > 
> > Of course. I didn't send the series, but thought you should see it. I
> > could have asked Mario to resend, but I thought a pointer would have
> > made it easy enough to find in your lkml folder, and it would avoid
> > splitting the conversation which resending would inevitably lead to. I
> > pruned this one because Christoph gets upset if I don't.
> > 
> > We can wait for v4 I guess. And next time I want to get your take on
> > something someone doesn't Cc you on, I'll just ask them to resend the
> > whole series with you on Cc.
> 
> Dude, that's the way it's always been, you know this!  Nothing new here,
> respining a patch series is normal, and asking people to review patch
> sets that you know already needs to be changed is strange.  Just do the
> fixes, and resend it, that's the simplest, quickest, and easiest thing
> to do for everyone involved.

Loud and clear Greg, won't happen again.

> Asking someone to review a patch based on a url just doesn't work, as
> how can I point out where the memory leak is exactly?  :)

As I said above, I provided the URL to make it easy to see the subjects,
senders, dates, etc. from the thread so you could find it in your LKML
folder and respond from there, not asking you to review from a webpage.
I do this from time to time when someone forgets to Cc me.

> Reviewers get thousands of emails a week (or a day in some cases), and
> making it as easy as possible for them to review the code is the key
> here.

I give this same message to people on a regular basis, and usually
include some of your load numbers to help drive the scale home. I guess
even those of us who know this are still prone to underestimating the
scale of the one:many developer:maintainer relationship.

Sorry for the lapse.

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI descriptor check

[Pali Rohár]

On Monday 02 October 2017 14:15:11 Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Pali Rohár [mailto:pali.rohar@gmail.com]
> > Sent: Saturday, September 30, 2017 3:01 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: dvhart@infradead.org; andy.shevchenko@gmail.com; linux-
> > kernel@vger.kernel.org; platform-driver-x86@vger.kernel.org; luto@kernel.org;
> > quasisec@google.com
> > Subject: Re: [PATCH v3 3/8] platform/x86: dell-wmi-smbios: Use Dell WMI
> > descriptor check
> > 
> > On Saturday 30 September 2017 21:48:39 Mario.Limonciello@dell.com wrote:
> > > > > +/*
> > > >
> > > > > + * Descriptor buffer is 128 byte long and contains:
> > > > ...
> > > >
> > > > > +	if (obj->buffer.length != 128) {
> > > > > +		dev_err(&wdev->dev,
> > > > > +			"Dell descriptor buffer has invalid length (%d)\n",
> > > > > +			obj->buffer.length);
> > > >
> > > > This seems odd. We call it an error (not a warning) if != 128, but
> > > > we only abort and return an error if it's < 16.
> > > >
> > > > If it's an error, we should return an error code, if anything above
> > > > 16 is acceptable but 128 is preferred, the above should be a
> > > > warning at best. (this scenario seems unlikely).
> > >
> > > Hopefully the original author can speak up to the intentions here.  I
> > > would feel that it should have errored out if it wasn't expected
> > > length too.
> > 
> > Code below access first 16 bytes of buffer. Therefore to prevent buffer
> > overflow check for 16 bytes is needed.
> > 
> > But IIRC we decided to do not throw error and continue driver loading
> > even when buffer length is not 128 (as expected by some Dell
> > documentation) as it could be possible regression because driver itself
> > does not depend on buffer length.
> > 
> 
> So I'm intending to change this in my next patch series.  I feel it should throw an
> error when the buffer length isn't 128.
> 
> My logic is that if you don't see the proper buffer size (or the proper header)
> then how can you trust that the rest of the data is reliable?  This means the format
> has changed or this isn't a real descriptor as expected by Dell (say some other vendor
> that has cloned the GUID).  It's better to abort in this situation.

Error handling now is up to you -- Dell. You know the best how your
API/ABI behave.

I did that change to be fully backward compatible with possibility to
read interface version number (needed for event handling logic).

> > > > > +		if (obj->buffer.length < 16) {
> > > > > +			ret = -EINVAL;
> > > > > +			goto out;
> > > > > +		}
> > > > > +	}
> > > > > +	desc_buffer = (u32 *)obj->buffer.pointer;
> > > > > +
> > > > > +	if (desc_buffer[0] != 0x4C4C4544 && desc_buffer[1] !=
> > > > > 0x494D5720)
> > 
> > --
> > Pali Rohár
> > pali.rohar@gmail.com

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

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg KH]

On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
> 
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call.  Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
> 
> When a WMI vendor driver declares a set of functions in a
> file_operations object the WMI bus driver will create a character
> device that maps to those file operations.
> 
> That character device will correspond to this path:
> /dev/wmi/$driver
> 
> This policy is selected as one driver may map and use multiple
> GUIDs and it would be better to only expose a single character
> device.
> 
> 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.

Ok, thanks to Darren, I've gone and dug these up while my boxes were
building stable kernels...

Why are you not just using the misc device interface here?  Why do you
need a whole new major and minor range?  Why not just register misc
devices dynamically as-needed?  Should be much simpler and easier to
maintain and reduce your code size a lot.

thanks,

greg k-h

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg KH]

On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> For WMI operations that are only Set or Query read or write sysfs
> attributes created by WMI vendor drivers make sense.
> 
> For other WMI operations that are run on Method, there needs to be a
> way to guarantee to userspace that the results from the method call
> belong to the data request to the method call.  Sysfs attributes don't
> work well in this scenario because two userspace processes may be
> competing at reading/writing an attribute and step on each other's
> data.
> 
> When a WMI vendor driver declares a set of functions in a
> file_operations object the WMI bus driver will create a character
> device that maps to those file operations.
> 
> That character device will correspond to this path:
> /dev/wmi/$driver
> 
> This policy is selected as one driver may map and use multiple
> GUIDs and it would be better to only expose a single character
> device.
> 
> 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>
> ---
>  drivers/platform/x86/wmi.c | 98 +++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/wmi.h        |  1 +
>  2 files changed, 94 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 4d73a87c2ddf..17399df87948 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -34,7 +34,9 @@
>  #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
>  
>  #include <linux/acpi.h>
> +#include <linux/cdev.h>
>  #include <linux/device.h>
> +#include <linux/idr.h>
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/list.h>
> @@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho");
>  MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
>  MODULE_LICENSE("GPL");
>  
> +#define WMI_MAX_DEVS  MINORMASK
> +static DEFINE_IDR(wmi_idr);

You never free the idr's memory when you unload the module :(

Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Greg KH]

On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> +{
> +	return nonseekable_open(inode, file);
> +}
> +
> +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> +{
> +	return 0;
> +}

Why even declare an open/release if you don't do anything with them?
Just leave them empty.

> +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> +	unsigned long arg)
> +{
> +	void __user *p = (void __user *) arg;
> +	size_t size;
> +	int ret = 0;
> +
> +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> +		return -ENOTTY;
> +
> +	switch (cmd) {
> +	case DELL_WMI_SMBIOS_CALL_CMD:
> +		size = sizeof(struct wmi_calling_interface_buffer);
> +		mutex_lock(&buffer_mutex);
> +		if (copy_from_user(devfs_buffer, p, size)) {
> +			ret = -EFAULT;
> +			goto fail_smbios_cmd;
> +		}
> +		ret = run_wmi_smbios_call(devfs_buffer);

You _are_ checking that your structures are valid here, right?  I didn't
see you really were, so I'll let you go audit your code paths for the
next set of patches.

But really, ugh, this seems horrible.  It's a huge vague data blob going
both ways, this feels ripe for abuse and other bad things.  Do you have
a working userspace implementation for all of this to publish at the
same time?

thanks,

greg k-h

RE: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, October 3, 2017 4:26 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
> Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> > +{
> > +	return nonseekable_open(inode, file);
> > +}
> > +
> > +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> > +{
> > +	return 0;
> > +}
> 
> Why even declare an open/release if you don't do anything with them?
> Just leave them empty.

Thanks, I'll modify that.

> 
> > +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> > +	unsigned long arg)
> > +{
> > +	void __user *p = (void __user *) arg;
> > +	size_t size;
> > +	int ret = 0;
> > +
> > +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> > +		return -ENOTTY;
> > +
> > +	switch (cmd) {
> > +	case DELL_WMI_SMBIOS_CALL_CMD:
> > +		size = sizeof(struct wmi_calling_interface_buffer);
> > +		mutex_lock(&buffer_mutex);
> > +		if (copy_from_user(devfs_buffer, p, size)) {
> > +			ret = -EFAULT;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		ret = run_wmi_smbios_call(devfs_buffer);
> 
> You _are_ checking that your structures are valid here, right?  I didn't
> see you really were, so I'll let you go audit your code paths for the
> next set of patches.

>From the Linux side there isn't much that can be done to verify the 
structure.  It doesn't contain a checksum, valid data that looks like a select
of 0 and class of 0 with a few input values looks the same as a few words
of the same length.

The validity of the structure is done by the system firmware.  If it's
invalid, appropriate return codes are set and consumers of it will be 
able to read that.

> 
> But really, ugh, this seems horrible.  It's a huge vague data blob going
> both ways, this feels ripe for abuse and other bad things.  Do you have
> a working userspace implementation for all of this to publish at the
> same time?
> 

Yes, I've got some support into fwupd and fwupdate that use this interface
already.  I expect the fwupdate PR will be ready to go at this same time,
but I'm working through some issues with fwupd still. 

RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, October 3, 2017 4:23 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
> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > For WMI operations that are only Set or Query read or write sysfs
> > attributes created by WMI vendor drivers make sense.
> >
> > For other WMI operations that are run on Method, there needs to be a
> > way to guarantee to userspace that the results from the method call
> > belong to the data request to the method call.  Sysfs attributes don't
> > work well in this scenario because two userspace processes may be
> > competing at reading/writing an attribute and step on each other's
> > data.
> >
> > When a WMI vendor driver declares a set of functions in a
> > file_operations object the WMI bus driver will create a character
> > device that maps to those file operations.
> >
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> >
> > This policy is selected as one driver may map and use multiple
> > GUIDs and it would be better to only expose a single character
> > device.
> >
> > 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.
> 
> Ok, thanks to Darren, I've gone and dug these up while my boxes were
> building stable kernels...
> 
> Why are you not just using the misc device interface here?  Why do you
> need a whole new major and minor range?  Why not just register misc
> devices dynamically as-needed?  Should be much simpler and easier to
> maintain and reduce your code size a lot.
> 

Thanks for this feedback.  I'll look into this as an alternative.

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Darren Hart]

On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote:
> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > For WMI operations that are only Set or Query read or write sysfs
> > attributes created by WMI vendor drivers make sense.
> > 
> > For other WMI operations that are run on Method, there needs to be a
> > way to guarantee to userspace that the results from the method call
> > belong to the data request to the method call.  Sysfs attributes don't
> > work well in this scenario because two userspace processes may be
> > competing at reading/writing an attribute and step on each other's
> > data.
> > 
> > When a WMI vendor driver declares a set of functions in a
> > file_operations object the WMI bus driver will create a character
> > device that maps to those file operations.
> > 
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> > 
> > This policy is selected as one driver may map and use multiple
> > GUIDs and it would be better to only expose a single character
> > device.
> > 
> > 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.
> 
> Ok, thanks to Darren, I've gone and dug these up while my boxes were
> building stable kernels...
> 
> Why are you not just using the misc device interface here?  Why do you
> need a whole new major and minor range?  Why not just register misc
> devices dynamically as-needed?  Should be much simpler and easier to
> maintain and reduce your code size a lot.

Thank you Greg, this simplifies things quite a bit.

Mario, the misc device interface will remove a lot of the boiler plate
setup and eliminate the need to allocate a new major number.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Mario.Limonciello]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Tuesday, October 3, 2017 4:24 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
> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> > For WMI operations that are only Set or Query read or write sysfs
> > attributes created by WMI vendor drivers make sense.
> >
> > For other WMI operations that are run on Method, there needs to be a
> > way to guarantee to userspace that the results from the method call
> > belong to the data request to the method call.  Sysfs attributes don't
> > work well in this scenario because two userspace processes may be
> > competing at reading/writing an attribute and step on each other's
> > data.
> >
> > When a WMI vendor driver declares a set of functions in a
> > file_operations object the WMI bus driver will create a character
> > device that maps to those file operations.
> >
> > That character device will correspond to this path:
> > /dev/wmi/$driver
> >
> > This policy is selected as one driver may map and use multiple
> > GUIDs and it would be better to only expose a single character
> > device.
> >
> > 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>
> > ---
> >  drivers/platform/x86/wmi.c | 98
> +++++++++++++++++++++++++++++++++++++++++++---
> >  include/linux/wmi.h        |  1 +
> >  2 files changed, 94 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> > index 4d73a87c2ddf..17399df87948 100644
> > --- a/drivers/platform/x86/wmi.c
> > +++ b/drivers/platform/x86/wmi.c
> > @@ -34,7 +34,9 @@
> >  #define pr_fmt(fmt)	KBUILD_MODNAME ": " fmt
> >
> >  #include <linux/acpi.h>
> > +#include <linux/cdev.h>
> >  #include <linux/device.h>
> > +#include <linux/idr.h>
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/list.h>
> > @@ -50,6 +52,9 @@ MODULE_AUTHOR("Carlos Corbacho");
> >  MODULE_DESCRIPTION("ACPI-WMI Mapping Driver");
> >  MODULE_LICENSE("GPL");
> >
> > +#define WMI_MAX_DEVS  MINORMASK
> > +static DEFINE_IDR(wmi_idr);
> 
> You never free the idr's memory when you unload the module :(

Whoops thanks, will fix. ☹

Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Darren Hart]

On Tue, Oct 03, 2017 at 11:26:11AM +0200, Greg KH wrote:
> On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> > +{
> > +	return nonseekable_open(inode, file);
> > +}
> > +
> > +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> > +{
> > +	return 0;
> > +}
> 
> Why even declare an open/release if you don't do anything with them?
> Just leave them empty.
> 
> > +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> > +	unsigned long arg)
> > +{
> > +	void __user *p = (void __user *) arg;
> > +	size_t size;
> > +	int ret = 0;
> > +
> > +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> > +		return -ENOTTY;
> > +
> > +	switch (cmd) {
> > +	case DELL_WMI_SMBIOS_CALL_CMD:
> > +		size = sizeof(struct wmi_calling_interface_buffer);
> > +		mutex_lock(&buffer_mutex);
> > +		if (copy_from_user(devfs_buffer, p, size)) {
> > +			ret = -EFAULT;
> > +			goto fail_smbios_cmd;
> > +		}
> > +		ret = run_wmi_smbios_call(devfs_buffer);
> 
> You _are_ checking that your structures are valid here, right?  I didn't
> see you really were, so I'll let you go audit your code paths for the
> next set of patches.
> 
> But really, ugh, this seems horrible.  It's a huge vague data blob going
> both ways, this feels ripe for abuse and other bad things.  Do you have
> a working userspace implementation for all of this to publish at the
> same time?
> 

This is the general challenge with WMI support, as it was designed to
provide userspace with a way to talk to the firmware.

For the more general case going forward the intent is to perform the MOF
parsing in the kernel, which will make it possible to do some automated
buffer validation.

This particular driver is an intermediate step improving on the
mechanism used for existing devices (libsmbios -> dcdbas -> SMM) to use
the safer WMI entry point (using an op region instead of passing a
physical memory pointer to SMM).

But, that said... Mario, in lieu of the MOF description of the buffer,
we should be able to do some driver specific validation of this buffer
here.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Tuesday, October 3, 2017 10:20 AM
> To: Greg KH <greg@kroah.com>
> Cc: 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
> Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Tue, Oct 03, 2017 at 11:26:11AM +0200, Greg KH wrote:
> > On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > > +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> > > +{
> > > +	return nonseekable_open(inode, file);
> > > +}
> > > +
> > > +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> > > +{
> > > +	return 0;
> > > +}
> >
> > Why even declare an open/release if you don't do anything with them?
> > Just leave them empty.
> >
> > > +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> > > +	unsigned long arg)
> > > +{
> > > +	void __user *p = (void __user *) arg;
> > > +	size_t size;
> > > +	int ret = 0;
> > > +
> > > +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> > > +		return -ENOTTY;
> > > +
> > > +	switch (cmd) {
> > > +	case DELL_WMI_SMBIOS_CALL_CMD:
> > > +		size = sizeof(struct wmi_calling_interface_buffer);
> > > +		mutex_lock(&buffer_mutex);
> > > +		if (copy_from_user(devfs_buffer, p, size)) {
> > > +			ret = -EFAULT;
> > > +			goto fail_smbios_cmd;
> > > +		}
> > > +		ret = run_wmi_smbios_call(devfs_buffer);
> >
> > You _are_ checking that your structures are valid here, right?  I didn't
> > see you really were, so I'll let you go audit your code paths for the
> > next set of patches.
> >
> > But really, ugh, this seems horrible.  It's a huge vague data blob going
> > both ways, this feels ripe for abuse and other bad things.  Do you have
> > a working userspace implementation for all of this to publish at the
> > same time?
> >
> 
> This is the general challenge with WMI support, as it was designed to
> provide userspace with a way to talk to the firmware.
> 
> For the more general case going forward the intent is to perform the MOF
> parsing in the kernel, which will make it possible to do some automated
> buffer validation.
> 
> This particular driver is an intermediate step improving on the
> mechanism used for existing devices (libsmbios -> dcdbas -> SMM) to use
> the safer WMI entry point (using an op region instead of passing a
> physical memory pointer to SMM).
> 
> But, that said... Mario, in lieu of the MOF description of the buffer,
> we should be able to do some driver specific validation of this buffer
> here.
> 

As the WMI interface exists today even the MOF doesn't describe the 
Content of the buffer either.  It's a buffer passed from userspace to BIOS and
back.  I described it as the struct, but I'm not really sure how that can
be further validated by the Linux kernel without a checksum.
This is no different than how dell-smbios and dcdbas operates.

New interfaces will have description that is parsable by MOF.

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Andy Lutomirski]

On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote:
> On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote:
>> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
>> > For WMI operations that are only Set or Query read or write sysfs
>> > attributes created by WMI vendor drivers make sense.
>> >
>> > For other WMI operations that are run on Method, there needs to be a
>> > way to guarantee to userspace that the results from the method call
>> > belong to the data request to the method call.  Sysfs attributes don't
>> > work well in this scenario because two userspace processes may be
>> > competing at reading/writing an attribute and step on each other's
>> > data.
>> >
>> > When a WMI vendor driver declares a set of functions in a
>> > file_operations object the WMI bus driver will create a character
>> > device that maps to those file operations.
>> >
>> > That character device will correspond to this path:
>> > /dev/wmi/$driver
>> >
>> > This policy is selected as one driver may map and use multiple
>> > GUIDs and it would be better to only expose a single character
>> > device.
>> >
>> > 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.
>>
>> Ok, thanks to Darren, I've gone and dug these up while my boxes were
>> building stable kernels...
>>
>> Why are you not just using the misc device interface here?  Why do you
>> need a whole new major and minor range?  Why not just register misc
>> devices dynamically as-needed?  Should be much simpler and easier to
>> maintain and reduce your code size a lot.
>
> Thank you Greg, this simplifies things quite a bit.
>
> Mario, the misc device interface will remove a lot of the boiler plate
> setup and eliminate the need to allocate a new major number.
>

In my mind, the problem with misc is that you may end up forever stuck
with a misc device, and they're distinct (visibly to userspace) from
all other character devices.

If you really want to be fancy, you could try to dust off a non-awful
character device API, a la:

https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&id=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Greg KH]

On Tue, Oct 03, 2017 at 09:48:05AM -0700, Andy Lutomirski wrote:
> On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote:
> >> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> >> > For WMI operations that are only Set or Query read or write sysfs
> >> > attributes created by WMI vendor drivers make sense.
> >> >
> >> > For other WMI operations that are run on Method, there needs to be a
> >> > way to guarantee to userspace that the results from the method call
> >> > belong to the data request to the method call.  Sysfs attributes don't
> >> > work well in this scenario because two userspace processes may be
> >> > competing at reading/writing an attribute and step on each other's
> >> > data.
> >> >
> >> > When a WMI vendor driver declares a set of functions in a
> >> > file_operations object the WMI bus driver will create a character
> >> > device that maps to those file operations.
> >> >
> >> > That character device will correspond to this path:
> >> > /dev/wmi/$driver
> >> >
> >> > This policy is selected as one driver may map and use multiple
> >> > GUIDs and it would be better to only expose a single character
> >> > device.
> >> >
> >> > 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.
> >>
> >> Ok, thanks to Darren, I've gone and dug these up while my boxes were
> >> building stable kernels...
> >>
> >> Why are you not just using the misc device interface here?  Why do you
> >> need a whole new major and minor range?  Why not just register misc
> >> devices dynamically as-needed?  Should be much simpler and easier to
> >> maintain and reduce your code size a lot.
> >
> > Thank you Greg, this simplifies things quite a bit.
> >
> > Mario, the misc device interface will remove a lot of the boiler plate
> > setup and eliminate the need to allocate a new major number.
> >
> 
> In my mind, the problem with misc is that you may end up forever stuck
> with a misc device, and they're distinct (visibly to userspace) from
> all other character devices.

What do you mean by "distinct"?  No one cares about the major number for
a device node anymore.

> If you really want to be fancy, you could try to dust off a non-awful
> character device API, a la:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&id=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7

Yeah, fixing this up and doing something like this has been on my "TODO"
for over a decade now :(

thanks,

greg k-h

RE: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Mario.Limonciello]

> -----Original Message-----
> From: Andy Lutomirski [mailto:luto@kernel.org]
> Sent: Tuesday, October 3, 2017 11:48 AM
> To: Darren Hart <dvhart@infradead.org>
> Cc: Greg KH <greg@kroah.com>; Limonciello, Mario
> <Mario_Limonciello@Dell.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>;
> Platform Driver <platform-driver-x86@vger.kernel.org>; Andy Lutomirski
> <luto@kernel.org>; quasisec@google.com; Pali Rohár <pali.rohar@gmail.com>
> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
> requested by drivers
> 
> On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote:
> > On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote:
> >> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
> >> > For WMI operations that are only Set or Query read or write sysfs
> >> > attributes created by WMI vendor drivers make sense.
> >> >
> >> > For other WMI operations that are run on Method, there needs to be a
> >> > way to guarantee to userspace that the results from the method call
> >> > belong to the data request to the method call.  Sysfs attributes don't
> >> > work well in this scenario because two userspace processes may be
> >> > competing at reading/writing an attribute and step on each other's
> >> > data.
> >> >
> >> > When a WMI vendor driver declares a set of functions in a
> >> > file_operations object the WMI bus driver will create a character
> >> > device that maps to those file operations.
> >> >
> >> > That character device will correspond to this path:
> >> > /dev/wmi/$driver
> >> >
> >> > This policy is selected as one driver may map and use multiple
> >> > GUIDs and it would be better to only expose a single character
> >> > device.
> >> >
> >> > 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.
> >>
> >> Ok, thanks to Darren, I've gone and dug these up while my boxes were
> >> building stable kernels...
> >>
> >> Why are you not just using the misc device interface here?  Why do you
> >> need a whole new major and minor range?  Why not just register misc
> >> devices dynamically as-needed?  Should be much simpler and easier to
> >> maintain and reduce your code size a lot.
> >
> > Thank you Greg, this simplifies things quite a bit.
> >
> > Mario, the misc device interface will remove a lot of the boiler plate
> > setup and eliminate the need to allocate a new major number.
> >
> 
> In my mind, the problem with misc is that you may end up forever stuck
> with a misc device, and they're distinct (visibly to userspace) from
> all other character devices.
> 
> If you really want to be fancy, you could try to dust off a non-awful
> character device API, a la:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&i
> d=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7

That's two years old.  What's the history of it?  Did you try to get it merged?

Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when requested by drivers

[Andy Lutomirski]

I think David Hermann had some review comments.

--Andy

On Oct 3, 2017, at 11:38 AM, <Mario.Limonciello@dell.com> <Mario.Limonciello@dell.com> wrote:

>> -----Original Message-----
>> From: Andy Lutomirski [mailto:luto@kernel.org]
>> Sent: Tuesday, October 3, 2017 11:48 AM
>> To: Darren Hart <dvhart@infradead.org>
>> Cc: Greg KH <greg@kroah.com>; Limonciello, Mario
>> <Mario_Limonciello@Dell.com>; Andy Shevchenko
>> <andy.shevchenko@gmail.com>; LKML <linux-kernel@vger.kernel.org>;
>> Platform Driver <platform-driver-x86@vger.kernel.org>; Andy Lutomirski
>> <luto@kernel.org>; quasisec@google.com; Pali Rohár <pali.rohar@gmail.com>
>> Subject: Re: [PATCH v3 4/8] platform/x86: wmi: create character devices when
>> requested by drivers
>> 
>>> On Tue, Oct 3, 2017 at 8:10 AM, Darren Hart <dvhart@infradead.org> wrote:
>>>> On Tue, Oct 03, 2017 at 11:23:23AM +0200, Greg KH wrote:
>>>>> On Wed, Sep 27, 2017 at 11:02:16PM -0500, Mario Limonciello wrote:
>>>>> For WMI operations that are only Set or Query read or write sysfs
>>>>> attributes created by WMI vendor drivers make sense.
>>>>> 
>>>>> For other WMI operations that are run on Method, there needs to be a
>>>>> way to guarantee to userspace that the results from the method call
>>>>> belong to the data request to the method call.  Sysfs attributes don't
>>>>> work well in this scenario because two userspace processes may be
>>>>> competing at reading/writing an attribute and step on each other's
>>>>> data.
>>>>> 
>>>>> When a WMI vendor driver declares a set of functions in a
>>>>> file_operations object the WMI bus driver will create a character
>>>>> device that maps to those file operations.
>>>>> 
>>>>> That character device will correspond to this path:
>>>>> /dev/wmi/$driver
>>>>> 
>>>>> This policy is selected as one driver may map and use multiple
>>>>> GUIDs and it would be better to only expose a single character
>>>>> device.
>>>>> 
>>>>> 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.
>>>> 
>>>> Ok, thanks to Darren, I've gone and dug these up while my boxes were
>>>> building stable kernels...
>>>> 
>>>> Why are you not just using the misc device interface here?  Why do you
>>>> need a whole new major and minor range?  Why not just register misc
>>>> devices dynamically as-needed?  Should be much simpler and easier to
>>>> maintain and reduce your code size a lot.
>>> 
>>> Thank you Greg, this simplifies things quite a bit.
>>> 
>>> Mario, the misc device interface will remove a lot of the boiler plate
>>> setup and eliminate the need to allocate a new major number.
>>> 
>> 
>> In my mind, the problem with misc is that you may end up forever stuck
>> with a misc device, and they're distinct (visibly to userspace) from
>> all other character devices.
>> 
>> If you really want to be fancy, you could try to dust off a non-awful
>> character device API, a la:
>> 
>> https://git.kernel.org/pub/scm/linux/kernel/git/luto/linux.git/commit/?h=u2f&i
>> d=d3ab93173d51cebf00dd2263fd0ce9f8cd6258f7
> 
> That's two years old.  What's the history of it?  Did you try to get it merged?

Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Darren Hart]

On Tue, Oct 03, 2017 at 03:49:04PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Tuesday, October 3, 2017 10:20 AM
> > To: Greg KH <greg@kroah.com>
> > Cc: 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
> > Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
> > device for userspace
> > 
> > On Tue, Oct 03, 2017 at 11:26:11AM +0200, Greg KH wrote:
> > > On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > > > +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> > > > +{
> > > > +	return nonseekable_open(inode, file);
> > > > +}
> > > > +
> > > > +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> > > > +{
> > > > +	return 0;
> > > > +}
> > >
> > > Why even declare an open/release if you don't do anything with them?
> > > Just leave them empty.
> > >
> > > > +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> > > > +	unsigned long arg)
> > > > +{
> > > > +	void __user *p = (void __user *) arg;
> > > > +	size_t size;
> > > > +	int ret = 0;
> > > > +
> > > > +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> > > > +		return -ENOTTY;
> > > > +
> > > > +	switch (cmd) {
> > > > +	case DELL_WMI_SMBIOS_CALL_CMD:
> > > > +		size = sizeof(struct wmi_calling_interface_buffer);
> > > > +		mutex_lock(&buffer_mutex);
> > > > +		if (copy_from_user(devfs_buffer, p, size)) {
> > > > +			ret = -EFAULT;
> > > > +			goto fail_smbios_cmd;
> > > > +		}
> > > > +		ret = run_wmi_smbios_call(devfs_buffer);
> > >
> > > You _are_ checking that your structures are valid here, right?  I didn't
> > > see you really were, so I'll let you go audit your code paths for the
> > > next set of patches.
> > >
> > > But really, ugh, this seems horrible.  It's a huge vague data blob going
> > > both ways, this feels ripe for abuse and other bad things.  Do you have
> > > a working userspace implementation for all of this to publish at the
> > > same time?
> > >
> > 
> > This is the general challenge with WMI support, as it was designed to
> > provide userspace with a way to talk to the firmware.
> > 
> > For the more general case going forward the intent is to perform the MOF
> > parsing in the kernel, which will make it possible to do some automated
> > buffer validation.
> > 
> > This particular driver is an intermediate step improving on the
> > mechanism used for existing devices (libsmbios -> dcdbas -> SMM) to use
> > the safer WMI entry point (using an op region instead of passing a
> > physical memory pointer to SMM).
> > 
> > But, that said... Mario, in lieu of the MOF description of the buffer,
> > we should be able to do some driver specific validation of this buffer
> > here.
> > 
> 
> As the WMI interface exists today even the MOF doesn't describe the 
> Content of the buffer either.  It's a buffer passed from userspace to BIOS and
> back.  I described it as the struct, but I'm not really sure how that can
> be further validated by the Linux kernel without a checksum.
> This is no different than how dell-smbios and dcdbas operates.

The notation about equivalence with the dell-smbios and dcdbas
implementation is a valid one. If all we do is move this to a WMI
managed access to SMBIOS and move away from the SMI based one, this is
still an improvement for difference in how memory is passed around as
described above.

I don't think a checksum will help with the concerns Greg is raising. A
userspace generated checksum only tells the kernel that what is in the
buffer is what userpsace intended. Similarly for the buffer coming back
from SMM.

Generally speaking, the kernel would want to validate data passing to
and from userspace and firmware - which WMI was designed to circumvent.

Mario:
Are there no specific bits that can be sanity checked? No reserved bits
in the buffer format? No Command Codes with a known range of values?

> New interfaces will have description that is parsable by MOF.

And to elaborate here. The above is about an improvement to the
dell-smbios driver for existing platforms which do not have available
MOF data. For platforms that do provide MOF data, we're working toward
moving the MOF parser into the kernel (rather than leaving it in
userspace as it was designed to be) which will afford the kernel more
oversight of these messages.

This series is an incremental improvement until the above is ready - but
it does lay out some of the ground work which we want to get agreement
on, mostly from patch 4 of 8 in the series:

1) The char dev IOCTL model for calling WMI methods

2) The /dev/wmi/<driver> path for the char dev

3) The function ops approach for WMI drivers to request the char dev
   from the WMI bus

-- 
Darren Hart
VMware Open Source Technology Center

Re: [PATCH v3 0/8] Introduce support for Dell SMBIOS over WMI

[Darren Hart]

On Sat, Sep 30, 2017 at 07:56:33PM +0000, Mario.Limonciello@dell.com wrote:
> > -----Original Message-----
> > From: Darren Hart [mailto:dvhart@infradead.org]
> > Sent: Friday, September 29, 2017 9:17 PM
> > To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> > Cc: 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
> > Subject: Re: [PATCH v3 0/8] Introduce support for Dell SMBIOS over WMI
> > 
> > On Wed, Sep 27, 2017 at 11:02:12PM -0500, Mario Limonciello wrote:

...

> > My other concern is the freeform structure around creating the file
> > operations in each driver for the chardev IOCTL. It seems like we need
> > some kind of defined mapping from METHOD index to IOCTL number, or else
> > some way to advertise what it is?
> > 
> 
> I was originally thinking it would be a good way to do this cleanly too, but my 
> main concern is this one character device may handle multiple methods problem.
> If you map method instance (index) to ioctl number you will most likely run into 
> clashes.  So maybe it's worth not allowing that?
> 
> So I've got another idea.  How about instead of a variety of freeform ioctl per driver,
> provide three ioctl functions for all the character devices that come in through
> the WMI bus to use (say IOC 'W') with either read, write or write/read.  The WMI bus
> should be able to know which driver to pass it on to by the character device used.
> 

This is not something I have any experience with. I see you added it to v4, so
let's see what others have to say there.

-- 
Darren Hart
VMware Open Source Technology Center

RE: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character device for userspace

[Mario.Limonciello]

> -----Original Message-----
> From: Darren Hart [mailto:dvhart@infradead.org]
> Sent: Wednesday, October 4, 2017 7:03 PM
> To: Limonciello, Mario <Mario_Limonciello@Dell.com>
> Cc: greg@kroah.com; 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
> Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce character
> device for userspace
> 
> On Tue, Oct 03, 2017 at 03:49:04PM +0000, Mario.Limonciello@dell.com wrote:
> > > -----Original Message-----
> > > From: Darren Hart [mailto:dvhart@infradead.org]
> > > Sent: Tuesday, October 3, 2017 10:20 AM
> > > To: Greg KH <greg@kroah.com>
> > > Cc: 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
> > > Subject: Re: [PATCH v3 5/8] platform/x86: dell-wmi-smbios: introduce
> character
> > > device for userspace
> > >
> > > On Tue, Oct 03, 2017 at 11:26:11AM +0200, Greg KH wrote:
> > > > On Wed, Sep 27, 2017 at 11:02:17PM -0500, Mario Limonciello wrote:
> > > > > +static int dell_wmi_smbios_open(struct inode *inode, struct file *file)
> > > > > +{
> > > > > +	return nonseekable_open(inode, file);
> > > > > +}
> > > > > +
> > > > > +static int dell_wmi_smbios_release(struct inode *inode, struct file *file)
> > > > > +{
> > > > > +	return 0;
> > > > > +}
> > > >
> > > > Why even declare an open/release if you don't do anything with them?
> > > > Just leave them empty.
> > > >
> > > > > +static long dell_wmi_smbios_ioctl(struct file *filp, unsigned int cmd,
> > > > > +	unsigned long arg)
> > > > > +{
> > > > > +	void __user *p = (void __user *) arg;
> > > > > +	size_t size;
> > > > > +	int ret = 0;
> > > > > +
> > > > > +	if (_IOC_TYPE(cmd) != DELL_WMI_SMBIOS_IOC)
> > > > > +		return -ENOTTY;
> > > > > +
> > > > > +	switch (cmd) {
> > > > > +	case DELL_WMI_SMBIOS_CALL_CMD:
> > > > > +		size = sizeof(struct wmi_calling_interface_buffer);
> > > > > +		mutex_lock(&buffer_mutex);
> > > > > +		if (copy_from_user(devfs_buffer, p, size)) {
> > > > > +			ret = -EFAULT;
> > > > > +			goto fail_smbios_cmd;
> > > > > +		}
> > > > > +		ret = run_wmi_smbios_call(devfs_buffer);
> > > >
> > > > You _are_ checking that your structures are valid here, right?  I didn't
> > > > see you really were, so I'll let you go audit your code paths for the
> > > > next set of patches.
> > > >
> > > > But really, ugh, this seems horrible.  It's a huge vague data blob going
> > > > both ways, this feels ripe for abuse and other bad things.  Do you have
> > > > a working userspace implementation for all of this to publish at the
> > > > same time?
> > > >
> > >
> > > This is the general challenge with WMI support, as it was designed to
> > > provide userspace with a way to talk to the firmware.
> > >
> > > For the more general case going forward the intent is to perform the MOF
> > > parsing in the kernel, which will make it possible to do some automated
> > > buffer validation.
> > >
> > > This particular driver is an intermediate step improving on the
> > > mechanism used for existing devices (libsmbios -> dcdbas -> SMM) to use
> > > the safer WMI entry point (using an op region instead of passing a
> > > physical memory pointer to SMM).
> > >
> > > But, that said... Mario, in lieu of the MOF description of the buffer,
> > > we should be able to do some driver specific validation of this buffer
> > > here.
> > >
> >
> > As the WMI interface exists today even the MOF doesn't describe the
> > Content of the buffer either.  It's a buffer passed from userspace to BIOS and
> > back.  I described it as the struct, but I'm not really sure how that can
> > be further validated by the Linux kernel without a checksum.
> > This is no different than how dell-smbios and dcdbas operates.
> 
> The notation about equivalence with the dell-smbios and dcdbas
> implementation is a valid one. If all we do is move this to a WMI
> managed access to SMBIOS and move away from the SMI based one, this is
> still an improvement for difference in how memory is passed around as
> described above.
> 
> I don't think a checksum will help with the concerns Greg is raising. A
> userspace generated checksum only tells the kernel that what is in the
> buffer is what userpsace intended. Similarly for the buffer coming back
> from SMM.
> 
> Generally speaking, the kernel would want to validate data passing to
> and from userspace and firmware - which WMI was designed to circumvent.
> 
> Mario:
> Are there no specific bits that can be sanity checked? No reserved bits
> in the buffer format? No Command Codes with a known range of values?
> 
Looking through the spec, I think I should be able to let the kernel check if 
the particular class is supported on the system when the call is made.  
Assuming that works I'll add something in v5.

This might also be useful for something like dell-laptop to check if the classes
its using are supported (and soften the blow up when new WMI interfaces
are introduced).

> > New interfaces will have description that is parsable by MOF.
> 
> And to elaborate here. The above is about an improvement to the
> dell-smbios driver for existing platforms which do not have available
> MOF data. For platforms that do provide MOF data, we're working toward
> moving the MOF parser into the kernel (rather than leaving it in
> userspace as it was designed to be) which will afford the kernel more
> oversight of these messages.
> 
> This series is an incremental improvement until the above is ready - but
> it does lay out some of the ground work which we want to get agreement
> on, mostly from patch 4 of 8 in the series:
> 
> 1) The char dev IOCTL model for calling WMI methods
> 
> 2) The /dev/wmi/<driver> path for the char dev
> 
> 3) The function ops approach for WMI drivers to request the char dev
>    from the WMI bus
> 
> --
> Darren Hart
> VMware Open Source Technology Center