linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
@ 2020-09-23 11:30 Divya Bharathi
  2020-09-23 17:11 ` Randy Dunlap
  2020-09-25 13:26 ` Hans de Goede
  0 siblings, 2 replies; 9+ messages in thread
From: Divya Bharathi @ 2020-09-23 11:30 UTC (permalink / raw)
  To: dvhart
  Cc: LKML, platform-driver-x86, Divya Bharathi, Hans de Goede,
	Andy Shevchenko, mark gross, Mario Limonciello, Prasanth KSR

The Dell WMI Systems Management Driver provides a sysfs
interface for systems management to enable BIOS configuration
capability on certain Dell Systems.

This driver allows user to configure Dell systems with a
uniform common interface. To facilitate this, the patch
introduces a generic way for driver to be able to create
configurable BIOS Attributes available in Setup (F2) screen.

Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: mark gross <mgross@linux.intel.com>

Co-developed-by: Mario Limonciello <mario.limonciello@dell.com>
Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
Co-developed-by: Prasanth KSR <prasanth.ksr@dell.com>
Signed-off-by: Prasanth KSR <prasanth.ksr@dell.com>
Signed-off-by: Divya Bharathi <divya.bharathi@dell.com>
---

Changes from v3 to v4:
 - Create a firmware-attributes class and tie ksets to a virtual device in it
 - Make modifier and value_modifier "dell only" attributes.
 - Correct some errors caught by kernel build bot around missing prototypes
 - Remove mutexes from populate_* functions and put in init_dell_bios_attrib_wmi instead
 - Move all code into a subdirectory drivers/platform/x86/dell-wmi-sysman and remove dell-wmi-*
   prefix on files
 - Move all data structures into shared struct
 - In alloc functions instead of kzalloc use kcalloc and check that there is no overflow
   + Same check for other alloc_foo-data functions
 -  populate_*: Move sysfs_create_group to end of the function to prevent race conditions
 - Save kernel object into each data instance and only remove that rather than sysfs_remove_group
 - Document in sysfs file what causes change uevents to come through
 - Only notify with change uevent one time on multiple settings modifications
 - Adjust lots of string handling
 - Make more objects static
 - Various whitespace corrections
 - Document map_wmi_error properly
 - Bump version to 5.11 (February 2021)

Changes from v2 to v3:
 - Fix a possible NULL pointer error in init
 - Add missing newlines to all dev_err/dev_dbg/pr_err/pr_debug statements
 - Correct updating passwords when both Admin and System password are set
 - Correct the WMI driver name
 - Correct some namespace clashing when compiled into the kernel (Reported by Mark Gross)
 - Correct some comment typos
 - Adopt suggestions made by Hans:
   + Use single global mutex
   + Clarify reason for uevents with a comment
   + Remove functions for set and get current password
   + Rename lower_bound to min_value and upper_bound to max_value
   + Rename possible_value to possible_values
   + Remove references to float
   + Build a separate passwords directory again since it behaves differently from the other
     attributes
   + Move more calls from pr_err -> dev_err
 - Documentation cleanups (see v2 patch feedback)
   + Grouping types
   + Syntax of `modifier` output

Changes from v1 to v2:
 - use pr_fmt instead of pr_err(DRIVER_NAME
 - re-order variables reverse xmas tree order
 - correct returns of -1 to error codes
 - correct usage of {} on some split line statements
 - Refine all documentation deficiencies suggested by Hans
 - Merge all attributes to a single directory
 - Overhaul WMI interface interaction as suggested by Hans
   * Move WMI driver registration to start of module
   * Remove usage of lists that only use first entry for WMI interfaces
   * Create a global structure shared across interface source files
   * Make get_current_password function static
   * Remove get_pending changes function, shared across global structure now.
- Overhaul use of mutexes
   * Make kset list mutex shared across source files
   * Remove unneeded dell-wmi-sysman call_mutex
   * Keep remaining call_mutexes in WMI functions
- Move security area calculation into a function
- Use NLS helper for utf8->utf16 conversion

 .../testing/sysfs-class-firmware-attributes   | 199 ++++++
 MAINTAINERS                                   |   8 +
 drivers/platform/x86/Kconfig                  |  12 +
 drivers/platform/x86/Makefile                 |   1 +
 drivers/platform/x86/dell-wmi-sysman/Makefile |   8 +
 .../x86/dell-wmi-sysman/biosattr-interface.c  | 199 ++++++
 .../x86/dell-wmi-sysman/dell-wmi-sysman.h     | 196 ++++++
 .../x86/dell-wmi-sysman/enum-attributes.c     | 188 ++++++
 .../x86/dell-wmi-sysman/int-attributes.c      | 169 +++++
 .../x86/dell-wmi-sysman/passobj-attributes.c  | 153 +++++
 .../dell-wmi-sysman/passwordattr-interface.c  | 167 +++++
 .../x86/dell-wmi-sysman/string-attributes.c   | 156 +++++
 drivers/platform/x86/dell-wmi-sysman/sysman.c | 589 ++++++++++++++++++
 13 files changed, 2045 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-firmware-attributes
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/Makefile
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/biosattr-interface.c
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/dell-wmi-sysman.h
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/enum-attributes.c
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/int-attributes.c
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/passobj-attributes.c
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/passwordattr-interface.c
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/string-attributes.c
 create mode 100644 drivers/platform/x86/dell-wmi-sysman/sysman.c

diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
new file mode 100644
index 000000000000..ad45d5717071
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
@@ -0,0 +1,199 @@
+What:		/sys/class/firmware-attributes/*/attributes/*/
+Date:		February 2021
+KernelVersion:	5.11
+Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
+		Mario Limonciello <mario.limonciello@dell.com>,
+		Prasanth KSR <prasanth.ksr@dell.com>
+Description:
+		A sysfs interface for systems management software to enable
+		configuration capability on supported systems.  This directory
+		exposes interfaces for interacting with configuration options.
+
+		Attributes can accept:
+		- a set of pre-defined valid values (enumeration)
+		- a range of numerical values (integer)
+		- a string
+
+		All attribute types support the following values:
+
+		current_value:	A file that can be read to obtain the current
+		value of the <attr>
+
+		This file can also be written to in order to update
+		the value of a <attr>
+
+		default_value:	A file that can be read to obtain the default
+		value of the <attr>
+
+		display_name:	A file that can be read to obtain a user friendly
+		description of the at <attr>
+
+		display_name_language_code:	A file that can be read to obtain
+		the IETF language tag corresponding to the "display_name" of the <attr>
+
+		"enumeration"-type specific properties:
+
+		possible_values: A file that can be read to obtain the possible
+		values of the <attr>. Values are separated using semi-colon.
+
+		"integer"-type specific properties:
+
+		min_value:	A file that can be read to obtain the lower
+		bound value of the <attr>
+
+		max_value:	A file that can be read to obtain the upper
+		bound value of the <attr>
+
+		scalar_increment:	A file that can be read to obtain the
+		resolution of the incremental value this attribute accepts.
+
+		"string"-type specific properties:
+
+		max_length:	A file that can be read to obtain the maximum
+		length value of the <attr>
+
+		min_length:	A file that can be read to obtain the minimum
+		length value of the <attr>
+
+		Dell specific class extensions
+		--------------------------
+
+		On Dell systems the following additional attributes are available:
+
+		dell_modifier: A file that can be read to obtain attribute-level
+		dependency rule. It says an attribute X will become read-only or
+		suppressed, if/if-not attribute Y is configured.
+
+		modifier rules can be in following format,
+		[ReadOnlyIf:<attribute>=<value>]
+		[ReadOnlyIfNot:<attribute>=<value>]
+		[SuppressIf:<attribute>=<value>]
+		[SuppressIfNot:<attribute>=<value>]
+
+		For example:
+		AutoOnFri/dell_modifier has value,
+			[SuppressIfNot:AutoOn=SelectDays]
+		This means AutoOnFri will be suppressed in BIOS setup if AutoOn
+		attribute is not "SelectDays" and its value will not be effective
+		through sysfs until this rule is met.
+
+		Enumeration attributes also support the following:
+
+		dell_value_modifier:	A file that can be read to obtain value-level
+		dependency. This file is similar to dell_modifier but here, an
+		attribute's current value will be forcefully changed based dependent
+		attributes value.
+
+		dell_value_modifier rules can be in following format,
+		<value>[ForceIf:<attribute>=<value>]
+		<value>[ForceIfNot:<attribute>=<value>]
+
+		For example,
+		LegacyOrom/dell_value_modifier has value,
+			Disabled[ForceIf:SecureBoot=Enabled]
+		This means LegacyOrom's current value will be forced to "Disabled"
+		in BIOS setup if SecureBoot is Enabled and its value will not be
+		effective through sysfs until this rule is met.
+
+What:		/sys/class/firmware-attributes/*/authentication/
+Date:		February 2021
+KernelVersion:	5.11
+Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
+		Mario Limonciello <mario.limonciello@dell.com>,
+		Prasanth KSR <prasanth.ksr@dell.com>
+
+		Devices support various authentication mechanisms which can be exposed
+		as a separate configuration object.
+
+		For example a "BIOS Admin" password and "System" Password can be set,
+		reset or cleared using these attributes.
+		- An "Admin" password is used for preventing modification to the BIOS
+		  settings.
+		- A "System" password is required to boot a machine.
+
+		Change in any of these two authentication methods will also generate an
+		uevent KOBJ_CHANGE.
+
+		is_authentication_set:	A file that can be read
+		to obtain flag to see if a password is set on <attr>
+
+		max_password_length:	A file that can be read to obtain the
+		maximum length of the Password
+
+		min_password_length:	A file that can be read to obtain the
+		minimum length of the Password
+
+		current_password: A write only value used for privileged access
+		such as setting attributes when a system or admin password is set
+		or resetting to a new password
+
+		new_password: A write only value that when used in tandem with
+		current_password will reset a system or admin password.
+
+		Note, password management is session specific. If Admin password is set,
+		same password must be written into current_password file (required for
+		password-validation) and must be cleared once the session is over.
+		For example:
+			echo "password" > current_password
+			echo "disabled" > TouchScreen/current_value
+			echo "" > current_password
+
+		Drivers may emit a CHANGE uevent when a password is set or unset
+		userspace may check it again.
+
+		On Dell systems, if Admin password is set, then all BIOS attributes
+		require password validation.
+
+What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
+Date:		February 2021
+KernelVersion:	5.11
+Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
+		Mario Limonciello <mario.limonciello@dell.com>,
+		Prasanth KSR <prasanth.ksr@dell.com>
+Description:
+		A read-only attribute reads 1 if a reboot is necessary to apply
+		pending BIOS attribute changes. Also, an uevent_KOBJ_CHANGE is
+		generated when it changes to 1.
+
+			0:	All BIOS attributes setting are current
+			1:	A reboot is necessary to get pending BIOS attribute changes
+				applied
+
+		Note, userspace applications need to follow below steps for efficient
+		BIOS management,
+		1.	Check if admin password is set. If yes, follow session method for
+			password management as briefed under authentication section above.
+		2.	Before setting any attribute, check if it has any modifiers
+			or value_modifiers. If yes, incorporate them and then modify
+			attribute.
+
+		Drivers may emit a CHANGE uevent when this value changes and userspace
+		may check it again.
+
+What:		/sys/class/firmware-attributes/*/attributes/reset_bios
+Date:		February 2021
+KernelVersion:	5.11
+Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
+		Mario Limonciello <mario.limonciello@dell.com>,
+		Prasanth KSR <prasanth.ksr@dell.com>
+Description:
+		This attribute can be used to reset the BIOS Configuration.
+		Specifically, it tells which type of reset BIOS configuration is being
+		requested on the host.
+
+		Reading from it returns a list of supported options encoded as:
+
+			'builtinsafe' (Built in safe configuration profile)
+			'lastknowngood' (Last known good saved configuration profile)
+			'factory' (Default factory settings configuration profile)
+			'custom' (Custom saved configuration profile)
+
+		The currently selected option is printed in square brackets as
+		shown below:
+
+		# echo "factory" > /sys/class/firmware-attributes/*/device/attributes/reset_bios
+		# cat /sys/class/firmware-attributes/*/device/attributes/reset_bios
+		# builtinsafe lastknowngood [factory] custom
+
+		Note that any changes to this attribute requires a reboot
+		for changes to take effect.
diff --git a/MAINTAINERS b/MAINTAINERS
index 9350506a1127..adc57e0f3d60 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4951,6 +4951,14 @@ M:	Mario Limonciello <mario.limonciello@dell.com>
 S:	Maintained
 F:	drivers/platform/x86/dell-wmi-descriptor.c
 
+DELL WMI SYSMAN DRIVER
+M:	Divya Bharathi <divya.bharathi@dell.com>
+M:	Mario Limonciello <mario.limonciello@dell.com>
+M:	Prasanth Ksr <prasanth.ksr@dell.com>
+L:	platform-driver-x86@vger.kernel.org
+S:	Maintained
+F:	drivers/platform/x86/dell-wmi-syman/*
+
 DELL WMI NOTIFICATIONS DRIVER
 M:	Matthew Garrett <mjg59@srcf.ucam.org>
 M:	Pali Rohár <pali@kernel.org>
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 40219bba6801..4fd7a3f0a904 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -430,6 +430,18 @@ config DELL_WMI
 	  To compile this driver as a module, choose M here: the module will
 	  be called dell-wmi.
 
+config DELL_WMI_SYSMAN
+	tristate "Dell WMI Systems management WMI driver"
+	depends on ACPI_WMI
+	depends on DMI
+	select NLS
+	help
+	  This driver allows changing BIOS settings on many Dell machines from
+	  2018 and newer without the use of any additional software.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called dell-wmi-sysman.
+
 config DELL_WMI_DESCRIPTOR
 	tristate
 	depends on ACPI_WMI
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 5f823f7eff45..36ce38e80c8f 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_DELL_WMI)			+= dell-wmi.o
 obj-$(CONFIG_DELL_WMI_DESCRIPTOR)	+= dell-wmi-descriptor.o
 obj-$(CONFIG_DELL_WMI_AIO)		+= dell-wmi-aio.o
 obj-$(CONFIG_DELL_WMI_LED)		+= dell-wmi-led.o
+obj-$(CONFIG_DELL_WMI_SYSMAN)		+= dell-wmi-sysman/
 
 # Fujitsu
 obj-$(CONFIG_AMILO_RFKILL)	+= amilo-rfkill.o
diff --git a/drivers/platform/x86/dell-wmi-sysman/Makefile b/drivers/platform/x86/dell-wmi-sysman/Makefile
new file mode 100644
index 000000000000..825fb2fbeea8
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/Makefile
@@ -0,0 +1,8 @@
+obj-$(CONFIG_DELL_WMI_SYSMAN)  += dell-wmi-sysman.o
+dell-wmi-sysman-objs := 	sysman.o		\
+				enum-attributes.o	\
+				int-attributes.o	\
+				string-attributes.o	\
+				passobj-attributes.o	\
+				biosattr-interface.o	\
+				passwordattr-interface.o
diff --git a/drivers/platform/x86/dell-wmi-sysman/biosattr-interface.c b/drivers/platform/x86/dell-wmi-sysman/biosattr-interface.c
new file mode 100644
index 000000000000..820c05d0fb8c
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/biosattr-interface.c
@@ -0,0 +1,199 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to SET methods under BIOS attributes interface GUID for use
+ * with dell-wmi-sysman
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#include <linux/nls.h>
+#include <linux/wmi.h>
+#include "dell-wmi-sysman.h"
+
+#define SETDEFAULTVALUES_METHOD_ID					0x02
+#define SETBIOSDEFAULTS_METHOD_ID					0x03
+#define SETATTRIBUTE_METHOD_ID						0x04
+
+static int call_biosattributes_interface(struct wmi_device *wdev, char *in_args, size_t size,
+					int method_id)
+{
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_buffer input;
+	union acpi_object *obj;
+	acpi_status status;
+	int ret = -EIO;
+
+	input.length =  (acpi_size) size;
+	input.pointer = in_args;
+	status = wmidev_evaluate_method(wdev, 0, method_id, &input, &output);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	obj = (union acpi_object *)output.pointer;
+	if (obj->type == ACPI_TYPE_INTEGER)
+		ret = obj->integer.value;
+
+	if (wmi_priv.pending_changes == 0) {
+		wmi_priv.pending_changes = 1;
+		/* let userland know it may need to check reboot pending again */
+		kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
+	}
+	kfree(output.pointer);
+	return map_wmi_error(ret);
+}
+
+/**
+ * set_attribute() - Update an attribute value
+ * @a_name: The attribute name
+ * @a_value: The attribute value
+ *
+ * Sets an attribute to new value
+ **/
+int set_attribute(const char *a_name, const char *a_value)
+{
+	size_t security_area_size, string_area_size, buffer_size;
+	char *attribute_name, *attribute_value;
+	u8 *name_len, *value_len;
+	char *buffer = NULL;
+	int ret;
+
+	mutex_lock(&wmi_priv.mutex);
+	if (!wmi_priv.bios_attr_wdev) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/* build/calculate buffer */
+	security_area_size = calculate_security_buffer();
+	string_area_size = (strlen(a_name) + strlen(a_value))*2;
+	buffer_size = security_area_size + string_area_size + sizeof(u16) * 2;
+	buffer = kzalloc(buffer_size, GFP_KERNEL);
+	if (!buffer) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* build security area */
+	if (strlen(wmi_priv.current_admin_password) > 0)
+		populate_security_buffer(buffer, wmi_priv.current_admin_password);
+
+	/* build variables to set */
+	name_len = buffer + security_area_size;
+	attribute_name = name_len + sizeof(u16);
+	*name_len = utf8s_to_utf16s(a_name, strlen(a_name), UTF16_HOST_ENDIAN,
+				    (wchar_t *) attribute_name, MAX_BUFF) * 2;
+	if (*name_len < 0) {
+		ret = -EINVAL;
+		dev_err(&wmi_priv.password_attr_wdev->dev, "UTF16 conversion failed\n");
+		goto out;
+	}
+
+	value_len = (u8 *) attribute_name + *name_len;
+	attribute_value = value_len + sizeof(u16);
+	*value_len = utf8s_to_utf16s(a_value, strlen(a_value), UTF16_HOST_ENDIAN,
+				    (wchar_t *) attribute_value, MAX_BUFF) * 2;
+	if (*value_len < 0) {
+		ret = -EINVAL;
+		dev_err(&wmi_priv.password_attr_wdev->dev, "UTF16 conversion failed\n");
+		goto out;
+	}
+
+	ret = call_biosattributes_interface(wmi_priv.bios_attr_wdev,
+					    buffer, buffer_size,
+					    SETATTRIBUTE_METHOD_ID);
+	if (ret == -EOPNOTSUPP)
+		dev_err(&wmi_priv.password_attr_wdev->dev, "admin password must be configured\n");
+	else if (ret == -EACCES)
+		dev_err(&wmi_priv.password_attr_wdev->dev, "invalid password\n");
+
+out:
+	kfree(buffer);
+	mutex_unlock(&wmi_priv.mutex);
+
+	return ret;
+}
+
+/**
+ * set_bios_defaults() - Resets BIOS defaults
+ * @deftype: the type of BIOS value reset to issue.
+ *
+ * Resets BIOS defaults
+ **/
+int set_bios_defaults(u8 deftype)
+{
+	size_t security_area_size, buffer_size;
+	size_t integer_area_size = sizeof(u8);
+	char *buffer = NULL;
+	u8 *defaultType;
+	int ret;
+
+	mutex_lock(&wmi_priv.mutex);
+	if (!wmi_priv.bios_attr_wdev) {
+		ret = -ENODEV;
+		goto out;
+	}
+
+	security_area_size = calculate_security_buffer();
+	buffer_size = security_area_size + integer_area_size;
+	buffer = kzalloc(buffer_size, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	/* build security area */
+	if (strlen(wmi_priv.current_admin_password) > 0)
+		populate_security_buffer(buffer, wmi_priv.current_admin_password);
+
+	defaultType = buffer + security_area_size;
+	*defaultType = deftype;
+
+	ret = call_biosattributes_interface(wmi_priv.bios_attr_wdev, buffer, buffer_size,
+					    SETBIOSDEFAULTS_METHOD_ID);
+	if (ret)
+		dev_err(&wmi_priv.bios_attr_wdev->dev, "reset BIOS defaults failed: %d\n", ret);
+
+out:
+	kfree(buffer);
+	mutex_unlock(&wmi_priv.mutex);
+
+	return ret;
+}
+
+static int bios_attr_set_interface_probe(struct wmi_device *wdev, const void *context)
+{
+	mutex_lock(&wmi_priv.mutex);
+	wmi_priv.bios_attr_wdev = wdev;
+	mutex_unlock(&wmi_priv.mutex);
+	return 0;
+}
+
+static int bios_attr_set_interface_remove(struct wmi_device *wdev)
+{
+	mutex_lock(&wmi_priv.mutex);
+	wmi_priv.bios_attr_wdev = NULL;
+	mutex_unlock(&wmi_priv.mutex);
+	return 0;
+}
+
+static const struct wmi_device_id bios_attr_set_interface_id_table[] = {
+	{ .guid_string = DELL_WMI_BIOS_ATTRIBUTES_INTERFACE_GUID },
+	{ },
+};
+static struct wmi_driver bios_attr_set_interface_driver = {
+	.driver = {
+		.name = DRIVER_NAME
+	},
+	.probe = bios_attr_set_interface_probe,
+	.remove = bios_attr_set_interface_remove,
+	.id_table = bios_attr_set_interface_id_table,
+};
+
+int init_bios_attr_set_interface(void)
+{
+	return wmi_driver_register(&bios_attr_set_interface_driver);
+}
+
+void exit_bios_attr_set_interface(void)
+{
+	wmi_driver_unregister(&bios_attr_set_interface_driver);
+}
+
+MODULE_DEVICE_TABLE(wmi, bios_attr_set_interface_id_table);
diff --git a/drivers/platform/x86/dell-wmi-sysman/dell-wmi-sysman.h b/drivers/platform/x86/dell-wmi-sysman/dell-wmi-sysman.h
new file mode 100644
index 000000000000..03e5299cf9b8
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/dell-wmi-sysman.h
@@ -0,0 +1,196 @@
+/* SPDX-License-Identifier: GPL-2.0
+ * Definitions for kernel modules using Dell WMI System Management Driver
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#ifndef _DELL_WMI_BIOS_ATTR_H_
+#define _DELL_WMI_BIOS_ATTR_H_
+
+#include <linux/wmi.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/capability.h>
+
+#define DRIVER_NAME					"dell-wmi-sysman"
+#define MAX_BUFF  512
+
+#define DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID	"F1DDEE52-063C-4784-A11E-8A06684B9BF5"
+#define DELL_WMI_BIOS_INTEGER_ATTRIBUTE_GUID		"F1DDEE52-063C-4784-A11E-8A06684B9BFA"
+#define DELL_WMI_BIOS_STRING_ATTRIBUTE_GUID		"F1DDEE52-063C-4784-A11E-8A06684B9BF9"
+#define DELL_WMI_BIOS_PASSOBJ_ATTRIBUTE_GUID		"0894B8D6-44A6-4719-97D7-6AD24108BFD4"
+#define DELL_WMI_BIOS_ATTRIBUTES_INTERFACE_GUID		"F1DDEE52-063C-4784-A11E-8A06684B9BF4"
+#define DELL_WMI_BIOS_PASSWORD_INTERFACE_GUID		"70FE8229-D03B-4214-A1C6-1F884B1A892A"
+
+struct enumeration_data {
+	struct kobject *attr_name_kobj;
+	char display_name_language_code[MAX_BUFF];
+	char attribute_name[MAX_BUFF];
+	char display_name[MAX_BUFF];
+	char default_value[MAX_BUFF];
+	char current_value[MAX_BUFF];
+	char dell_modifier[MAX_BUFF];
+	int value_modifier_count;
+	char dell_value_modifier[MAX_BUFF];
+	int possible_values_count;
+	char possible_values[MAX_BUFF];
+	char type[MAX_BUFF];
+};
+
+struct integer_data {
+	struct kobject *attr_name_kobj;
+	char display_name_language_code[MAX_BUFF];
+	char attribute_name[MAX_BUFF];
+	char display_name[MAX_BUFF];
+	int default_value;
+	int current_value;
+	char dell_modifier[MAX_BUFF];
+	int min_value;
+	int max_value;
+	int scalar_increment;
+	char type[MAX_BUFF];
+};
+
+struct str_data {
+	struct kobject *attr_name_kobj;
+	char display_name_language_code[MAX_BUFF];
+	char attribute_name[MAX_BUFF];
+	char display_name[MAX_BUFF];
+	char default_value[MAX_BUFF];
+	char current_value[MAX_BUFF];
+	char dell_modifier[MAX_BUFF];
+	int min_length;
+	int max_length;
+	char type[MAX_BUFF];
+};
+
+struct po_data {
+	struct kobject *attr_name_kobj;
+	char attribute_name[MAX_BUFF];
+	int is_authentication_set;
+	int min_password_length;
+	int max_password_length;
+	char type[MAX_BUFF];
+};
+
+struct wmi_sysman_priv {
+	char current_admin_password[MAX_BUFF];
+	char current_system_password[MAX_BUFF];
+	struct wmi_device *password_attr_wdev;
+	struct wmi_device *bios_attr_wdev;
+	struct kset *authentication_dir_kset;
+	struct kset *main_dir_kset;
+	struct enumeration_data *enumeration_data;
+	int enumeration_instances_count;
+	struct integer_data *integer_data;
+	int integer_instances_count;
+	struct str_data *str_data;
+	int str_instances_count;
+	struct po_data *po_data;
+	int po_instances_count;
+	bool pending_changes;
+	struct mutex mutex;
+};
+
+/* global structure used by multiple WMI interfaces */
+extern struct wmi_sysman_priv wmi_priv;
+
+enum { ENUM, INT, STR, PO };
+
+enum {
+	ATTR_NAME,
+	DISPL_NAME_LANG_CODE,
+	DISPLAY_NAME,
+	DEFAULT_VAL,
+	CURRENT_VAL,
+	MODIFIER
+};
+
+#define get_instance_id(type)							\
+int get_##type##_instance_id(struct kobject *kobj);				\
+int get_##type##_instance_id(struct kobject *kobj)				\
+{										\
+	int i;									\
+	for (i = 0; i <= wmi_priv.type##_instances_count; i++) {		\
+		if (!(strcmp(kobj->name, wmi_priv.type##_data[i].attribute_name)))\
+			return i;						\
+	}									\
+	return -EIO;								\
+}
+
+#define attribute_s_property_show(name, type)					\
+static ssize_t name##_show(struct kobject *kobj, struct kobj_attribute *attr,	\
+			   char *buf)						\
+{										\
+	int i = get_##type##_instance_id(kobj);					\
+	if (i >= 0)								\
+		return sprintf(buf, "%s\n", wmi_priv.type##_data[i].name);	\
+	return 0;								\
+}
+
+#define attribute_n_property_show(name, type)					\
+static ssize_t name##_show(struct kobject *kobj, struct kobj_attribute *attr,	\
+			   char *buf)						\
+{										\
+	int i = get_##type##_instance_id(kobj);					\
+	if (i >= 0)								\
+		return sprintf(buf, "%d\n", wmi_priv.type##_data[i].name);	\
+	return 0;								\
+}
+
+#define attribute_property_store(curr_val, type)				\
+static ssize_t curr_val##_store(struct kobject *kobj,				\
+				struct kobj_attribute *attr,			\
+				const char *buf, size_t count)			\
+{										\
+	char *p = memchr(buf, '\n', count);					\
+	int ret = -EIO;								\
+	int i;									\
+										\
+	if (p != NULL)								\
+		*p = '\0';							\
+	i = get_##type##_instance_id(kobj);					\
+	if (i >= 0)								\
+		ret = validate_##type##_input(i, buf);				\
+	if (!ret)								\
+		ret = set_attribute(kobj->name, buf);				\
+	return ret ? ret : count;						\
+}
+
+union acpi_object *get_wmiobj_pointer(int instance_id, const char *guid_string);
+int get_instance_count(const char *guid_string);
+void strlcpy_attr(char *dest, char *src);
+
+int populate_enum_data(union acpi_object *enumeration_obj, int instance_id,
+			struct kobject *attr_name_kobj);
+int alloc_enum_data(void);
+void exit_enum_attributes(void);
+
+int populate_int_data(union acpi_object *integer_obj, int instance_id,
+			struct kobject *attr_name_kobj);
+int alloc_int_data(void);
+void exit_int_attributes(void);
+
+int populate_str_data(union acpi_object *str_obj, int instance_id, struct kobject *attr_name_kobj);
+int alloc_str_data(void);
+void exit_str_attributes(void);
+
+int populate_po_data(union acpi_object *po_obj, int instance_id, struct kobject *attr_name_kobj);
+int alloc_po_data(void);
+void exit_po_attributes(void);
+
+int set_attribute(const char *a_name, const char *a_value);
+int set_bios_defaults(u8 defType);
+
+void exit_bios_attr_set_interface(void);
+int init_bios_attr_set_interface(void);
+int map_wmi_error(int error_code);
+size_t calculate_security_buffer(void);
+void populate_security_buffer(char *buffer, char *authentication);
+
+int set_new_password(const char *password_type, const char *new);
+int init_bios_attr_pass_interface(void);
+void exit_bios_attr_pass_interface(void);
+
+#endif
diff --git a/drivers/platform/x86/dell-wmi-sysman/enum-attributes.c b/drivers/platform/x86/dell-wmi-sysman/enum-attributes.c
new file mode 100644
index 000000000000..cfcb226db634
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/enum-attributes.c
@@ -0,0 +1,188 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to enumeration type attributes under BIOS Enumeration GUID for use
+ * with dell-wmi-sysman
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#include "dell-wmi-sysman.h"
+
+get_instance_id(enumeration);
+
+static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	int instance_id;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	instance_id = get_enumeration_instance_id(kobj);
+	if (instance_id >= 0) {
+		union acpi_object *obj;
+
+		/* need to use specific instance_id and guid combination to get right data */
+		obj = get_wmiobj_pointer(instance_id, DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
+		if (!obj)
+			return -AE_ERROR;
+		strlcpy_attr(wmi_priv.enumeration_data[instance_id].current_value,
+		       obj->package.elements[CURRENT_VAL].string.pointer);
+		kfree(obj);
+		return sprintf(buf, "%s\n", wmi_priv.enumeration_data[instance_id].current_value);
+	}
+	return -EIO;
+}
+
+/**
+ * validate_enumeration_input() - Validate input of current_value against possible values
+ * @instance_id: The instance on which input is validated
+ * @buf: Input value
+ **/
+static int validate_enumeration_input(int instance_id, const char *buf)
+{
+	char *options, *tmp, *p;
+	int ret = -EINVAL;
+
+	options = tmp = kstrdup(wmi_priv.enumeration_data[instance_id].possible_values,
+				 GFP_KERNEL);
+	if (!options)
+		return -ENOMEM;
+
+	while ((p = strsep(&options, ";")) != NULL) {
+		if (!*p)
+			continue;
+		if (!strncasecmp(p, buf, strlen(p))) {
+			ret = 0;
+			break;
+		}
+	}
+
+	kfree(tmp);
+	return ret;
+}
+
+attribute_s_property_show(display_name_language_code, enumeration);
+static struct kobj_attribute displ_langcode =
+		__ATTR_RO(display_name_language_code);
+
+attribute_s_property_show(display_name, enumeration);
+static struct kobj_attribute displ_name =
+		__ATTR_RO(display_name);
+
+attribute_s_property_show(default_value, enumeration);
+static struct kobj_attribute default_val =
+		__ATTR_RO(default_value);
+
+attribute_property_store(current_value, enumeration);
+static struct kobj_attribute current_val =
+		__ATTR_RW(current_value);
+
+attribute_s_property_show(dell_modifier, enumeration);
+static struct kobj_attribute modifier =
+		__ATTR_RO(dell_modifier);
+
+attribute_s_property_show(dell_value_modifier, enumeration);
+static struct kobj_attribute value_modfr =
+		__ATTR_RO(dell_value_modifier);
+
+attribute_s_property_show(possible_values, enumeration);
+static struct kobj_attribute poss_val =
+		__ATTR_RO(possible_values);
+
+attribute_s_property_show(type, enumeration);
+static struct kobj_attribute type =
+		__ATTR_RO(type);
+
+static struct attribute *enumeration_attrs[] = {
+	&displ_langcode.attr,
+	&displ_name.attr,
+	&default_val.attr,
+	&current_val.attr,
+	&modifier.attr,
+	&value_modfr.attr,
+	&poss_val.attr,
+	&type.attr,
+	NULL,
+};
+
+static const struct attribute_group enumeration_attr_group = {
+	.attrs = enumeration_attrs,
+};
+
+int alloc_enum_data(void)
+{
+	int ret = 0;
+
+	wmi_priv.enumeration_instances_count =
+		get_instance_count(DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
+	wmi_priv.enumeration_data = kcalloc(wmi_priv.enumeration_instances_count,
+					sizeof(struct enumeration_data), GFP_KERNEL);
+	if (!wmi_priv.enumeration_data)
+		ret = -ENOMEM;
+	return ret;
+}
+
+/**
+ * populate_enum_data() - Populate all properties of an instance under enumeration attribute
+ * @enumeration_obj: ACPI object with enumeration data
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ **/
+int populate_enum_data(union acpi_object *enumeration_obj, int instance_id,
+			struct kobject *attr_name_kobj)
+{
+	int i, next_obj;
+
+	wmi_priv.enumeration_data[instance_id].attr_name_kobj = attr_name_kobj;
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].attribute_name,
+		enumeration_obj[ATTR_NAME].string.pointer);
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].display_name_language_code,
+		enumeration_obj[DISPL_NAME_LANG_CODE].string.pointer);
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].display_name,
+		enumeration_obj[DISPLAY_NAME].string.pointer);
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].default_value,
+		enumeration_obj[DEFAULT_VAL].string.pointer);
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].current_value,
+		enumeration_obj[CURRENT_VAL].string.pointer);
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].dell_modifier,
+		enumeration_obj[MODIFIER].string.pointer);
+
+	next_obj = MODIFIER + 1;
+
+	wmi_priv.enumeration_data[instance_id].value_modifier_count =
+		(uintptr_t)enumeration_obj[next_obj].string.pointer;
+
+	for (i = 0; i < wmi_priv.enumeration_data[instance_id].value_modifier_count; i++) {
+		strcat(wmi_priv.enumeration_data[instance_id].dell_value_modifier,
+			enumeration_obj[++next_obj].string.pointer);
+		strcat(wmi_priv.enumeration_data[instance_id].dell_value_modifier, ";");
+	}
+
+	wmi_priv.enumeration_data[instance_id].possible_values_count =
+		(uintptr_t) enumeration_obj[++next_obj].string.pointer;
+
+	for (i = 0; i < wmi_priv.enumeration_data[instance_id].possible_values_count; i++) {
+		strcat(wmi_priv.enumeration_data[instance_id].possible_values,
+			enumeration_obj[++next_obj].string.pointer);
+		strcat(wmi_priv.enumeration_data[instance_id].possible_values, ";");
+	}
+	strlcpy_attr(wmi_priv.enumeration_data[instance_id].type, "enumeration");
+
+	return sysfs_create_group(attr_name_kobj, &enumeration_attr_group);
+}
+
+/**
+ * exit_enum_attributes() - Clear all attribute data
+ *
+ * Clears all data allocated for this group of attributes
+ **/
+void exit_enum_attributes(void)
+{
+	int instance_id;
+
+	for (instance_id = 0; instance_id < wmi_priv.enumeration_instances_count; instance_id++) {
+		if (wmi_priv.enumeration_data[instance_id].attr_name_kobj)
+			sysfs_remove_group(wmi_priv.enumeration_data[instance_id].attr_name_kobj,
+								&enumeration_attr_group);
+	}
+	kfree(wmi_priv.enumeration_data);
+}
diff --git a/drivers/platform/x86/dell-wmi-sysman/int-attributes.c b/drivers/platform/x86/dell-wmi-sysman/int-attributes.c
new file mode 100644
index 000000000000..f668d69eaf09
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/int-attributes.c
@@ -0,0 +1,169 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to integer type attributes under BIOS Integer GUID for use with
+ * dell-wmi-sysman
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#include "dell-wmi-sysman.h"
+
+enum int_properties {MIN_VALUE = 6, MAX_VALUE, SCALAR_INCR};
+
+get_instance_id(integer);
+
+static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	int instance_id;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	instance_id = get_integer_instance_id(kobj);
+	if (instance_id >= 0) {
+		union acpi_object *obj;
+
+		/* need to use specific instance_id and guid combination to get right data */
+		obj = get_wmiobj_pointer(instance_id, DELL_WMI_BIOS_INTEGER_ATTRIBUTE_GUID);
+		if (!obj)
+			return -AE_ERROR;
+		wmi_priv.integer_data[instance_id].current_value =
+			(uintptr_t)obj->package.elements[CURRENT_VAL].string.pointer;
+		kfree(obj);
+		return sprintf(buf, "%d\n", wmi_priv.integer_data[instance_id].current_value);
+	}
+	return -EIO;
+}
+
+/**
+ * validate_integer_input() - Validate input of current_value against lower and upper bound
+ * @instance_id: The instance on which input is validated
+ * @buf: Input value
+ **/
+static int validate_integer_input(int instance_id, const char *buf)
+{
+	int ret = -EINVAL;
+	int in_val;
+
+	if (kstrtoint(buf, 0, &in_val))
+		return ret;
+	if ((in_val >= wmi_priv.integer_data[instance_id].min_value) &&
+	(in_val <= wmi_priv.integer_data[instance_id].max_value))
+		ret = 0;
+
+	return ret;
+}
+
+attribute_s_property_show(display_name_language_code, integer);
+static struct kobj_attribute integer_displ_langcode =
+	__ATTR_RO(display_name_language_code);
+
+attribute_s_property_show(display_name, integer);
+static struct kobj_attribute integer_displ_name =
+	__ATTR_RO(display_name);
+
+attribute_n_property_show(default_value, integer);
+static struct kobj_attribute integer_default_val =
+	__ATTR_RO(default_value);
+
+attribute_property_store(current_value, integer);
+static struct kobj_attribute integer_current_val =
+	__ATTR_RW(current_value);
+
+attribute_s_property_show(dell_modifier, integer);
+static struct kobj_attribute integer_modifier =
+	__ATTR_RO(dell_modifier);
+
+attribute_n_property_show(min_value, integer);
+static struct kobj_attribute integer_lower_bound =
+	__ATTR_RO(min_value);
+
+attribute_n_property_show(max_value, integer);
+static struct kobj_attribute integer_upper_bound =
+	__ATTR_RO(max_value);
+
+attribute_n_property_show(scalar_increment, integer);
+static struct kobj_attribute integer_scalar_increment =
+	__ATTR_RO(scalar_increment);
+
+attribute_s_property_show(type, integer);
+static struct kobj_attribute integer_type =
+	__ATTR_RO(type);
+
+static struct attribute *integer_attrs[] = {
+	&integer_displ_langcode.attr,
+	&integer_displ_name.attr,
+	&integer_default_val.attr,
+	&integer_current_val.attr,
+	&integer_modifier.attr,
+	&integer_lower_bound.attr,
+	&integer_upper_bound.attr,
+	&integer_scalar_increment.attr,
+	&integer_type.attr,
+	NULL,
+};
+
+static const struct attribute_group integer_attr_group = {
+	.attrs = integer_attrs,
+};
+
+int alloc_int_data(void)
+{
+	int ret = 0;
+
+	wmi_priv.integer_instances_count = get_instance_count(DELL_WMI_BIOS_INTEGER_ATTRIBUTE_GUID);
+	wmi_priv.integer_data = kcalloc(wmi_priv.integer_instances_count,
+					sizeof(struct integer_data), GFP_KERNEL);
+	if (!wmi_priv.integer_data)
+		ret = -ENOMEM;
+	return ret;
+}
+
+/**
+ * populate_int_data() - Populate all properties of an instance under integer attribute
+ * @integer_obj: ACPI object with integer data
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ **/
+int populate_int_data(union acpi_object *integer_obj, int instance_id,
+			struct kobject *attr_name_kobj)
+{
+	wmi_priv.integer_data[instance_id].attr_name_kobj = attr_name_kobj;
+	strlcpy_attr(wmi_priv.integer_data[instance_id].attribute_name,
+		integer_obj[ATTR_NAME].string.pointer);
+	strlcpy_attr(wmi_priv.integer_data[instance_id].display_name_language_code,
+		integer_obj[DISPL_NAME_LANG_CODE].string.pointer);
+	strlcpy_attr(wmi_priv.integer_data[instance_id].display_name,
+		integer_obj[DISPLAY_NAME].string.pointer);
+	wmi_priv.integer_data[instance_id].default_value =
+		(uintptr_t)integer_obj[DEFAULT_VAL].string.pointer;
+	wmi_priv.integer_data[instance_id].current_value =
+		(uintptr_t)integer_obj[CURRENT_VAL].string.pointer;
+	strlcpy_attr(wmi_priv.integer_data[instance_id].dell_modifier,
+		integer_obj[MODIFIER].string.pointer);
+	wmi_priv.integer_data[instance_id].min_value =
+		(uintptr_t)integer_obj[MIN_VALUE].string.pointer;
+	wmi_priv.integer_data[instance_id].max_value =
+		(uintptr_t)integer_obj[MAX_VALUE].string.pointer;
+	wmi_priv.integer_data[instance_id].scalar_increment =
+		(uintptr_t)integer_obj[SCALAR_INCR].string.pointer;
+	strlcpy_attr(wmi_priv.integer_data[instance_id].type, "integer");
+
+	return sysfs_create_group(attr_name_kobj, &integer_attr_group);
+}
+
+/**
+ * exit_int_attributes() - Clear all attribute data
+ *
+ * Clears all data allocated for this group of attributes
+ **/
+void exit_int_attributes(void)
+{
+	int instance_id;
+
+	for (instance_id = 0; instance_id < wmi_priv.integer_instances_count; instance_id++) {
+		if (wmi_priv.integer_data[instance_id].attr_name_kobj)
+			sysfs_remove_group(wmi_priv.integer_data[instance_id].attr_name_kobj,
+								&integer_attr_group);
+	}
+	kfree(wmi_priv.integer_data);
+}
diff --git a/drivers/platform/x86/dell-wmi-sysman/passobj-attributes.c b/drivers/platform/x86/dell-wmi-sysman/passobj-attributes.c
new file mode 100644
index 000000000000..d02c123c0ab1
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/passobj-attributes.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to password object type attributes under BIOS Password Object GUID for
+ * use with dell-wmi-sysman
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#include "dell-wmi-sysman.h"
+
+enum po_properties {IS_PASS_SET = 1, MIN_PASS_LEN, MAX_PASS_LEN};
+
+get_instance_id(po);
+
+static ssize_t is_authentication_set_show(struct kobject *kobj, struct kobj_attribute *attr,
+					  char *buf)
+{
+	int instance_id = get_po_instance_id(kobj);
+
+	if (instance_id >= 0) {
+		union acpi_object *obj;
+
+		/* need to use specific instance_id and guid combination to get right data */
+		obj = get_wmiobj_pointer(instance_id, DELL_WMI_BIOS_PASSOBJ_ATTRIBUTE_GUID);
+		if (!obj)
+			return -AE_ERROR;
+		wmi_priv.po_data[instance_id].is_authentication_set =
+			(uintptr_t)obj->package.elements[IS_PASS_SET].string.pointer;
+		kfree(obj);
+		return sprintf(buf, "%d\n", wmi_priv.po_data[instance_id].is_authentication_set);
+	}
+	return -EIO;
+}
+
+struct kobj_attribute po_is_pass_set =
+		__ATTR_RO(is_authentication_set);
+
+static ssize_t current_password_store(struct kobject *kobj,
+				      struct kobj_attribute *attr,
+				      const char *buf, size_t count)
+{
+	char *p = memchr(buf, '\n', count);
+	int ret;
+
+	if (p != NULL)
+		*p = '\0';
+	if (strlen(buf) >= MAX_BUFF)
+		return -EINVAL;
+
+	if (strcmp(kobj->name, "Admin") == 0)
+		memcpy(wmi_priv.current_admin_password, buf, (strlen(buf) + 1));
+	if (strcmp(kobj->name, "System") == 0)
+		memcpy(wmi_priv.current_system_password, buf, (strlen(buf) + 1));
+	return ret ? ret : count;
+}
+
+struct kobj_attribute po_current_password =
+		__ATTR_WO(current_password);
+
+static ssize_t new_password_store(struct kobject *kobj,
+				  struct kobj_attribute *attr,
+				  const char *buf, size_t count)
+{
+	char *p = memchr(buf, '\n', count);
+	int ret;
+
+	if (p != NULL)
+		*p = '\0';
+	if (strlen(buf) > MAX_BUFF)
+		return -EINVAL;
+
+	ret = set_new_password(kobj->name, buf);
+	return ret ? ret : count;
+}
+
+struct kobj_attribute po_new_password =
+		__ATTR_WO(new_password);
+
+attribute_n_property_show(min_password_length, po);
+struct kobj_attribute po_min_pass_length =
+		__ATTR_RO(min_password_length);
+
+attribute_n_property_show(max_password_length, po);
+struct kobj_attribute po_max_pass_length =
+		__ATTR_RO(max_password_length);
+
+attribute_s_property_show(type, po);
+struct kobj_attribute po_type =
+	__ATTR_RO(type);
+
+static struct attribute *po_attrs[] = {
+	&po_is_pass_set.attr,
+	&po_min_pass_length.attr,
+	&po_max_pass_length.attr,
+	&po_current_password.attr,
+	&po_new_password.attr,
+	&po_type.attr,
+	NULL,
+};
+
+static const struct attribute_group po_attr_group = {
+	.attrs = po_attrs,
+};
+
+int alloc_po_data(void)
+{
+	int ret = 0;
+
+	wmi_priv.po_instances_count = get_instance_count(DELL_WMI_BIOS_PASSOBJ_ATTRIBUTE_GUID);
+	wmi_priv.po_data = kcalloc(wmi_priv.po_instances_count, sizeof(struct po_data), GFP_KERNEL);
+	if (!wmi_priv.po_data)
+		ret = -ENOMEM;
+	return ret;
+}
+
+/**
+ * populate_po_data() - Populate all properties of an instance under password object attribute
+ * @po_obj: ACPI object with password object data
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ **/
+int populate_po_data(union acpi_object *po_obj, int instance_id, struct kobject *attr_name_kobj)
+{
+	wmi_priv.po_data[instance_id].attr_name_kobj = attr_name_kobj;
+	strlcpy_attr(wmi_priv.po_data[instance_id].attribute_name,
+		     po_obj[ATTR_NAME].string.pointer);
+	wmi_priv.po_data[instance_id].is_authentication_set =
+		(uintptr_t)po_obj[IS_PASS_SET].string.pointer;
+	wmi_priv.po_data[instance_id].min_password_length =
+		(uintptr_t)po_obj[MIN_PASS_LEN].string.pointer;
+	wmi_priv.po_data[instance_id].max_password_length =
+		(uintptr_t) po_obj[MAX_PASS_LEN].string.pointer;
+	strlcpy_attr(wmi_priv.po_data[instance_id].type, "password_object");
+
+	return sysfs_create_group(attr_name_kobj, &po_attr_group);
+}
+
+/**
+ * exit_po_attributes() - Clear all attribute data
+ *
+ * Clears all data allocated for this group of attributes
+ **/
+void exit_po_attributes(void)
+{
+	int instance_id;
+
+	for (instance_id = 0; instance_id < wmi_priv.po_instances_count; instance_id++) {
+		if (wmi_priv.po_data[instance_id].attr_name_kobj)
+			sysfs_remove_group(wmi_priv.po_data[instance_id].attr_name_kobj,
+								&po_attr_group);
+	}
+	kfree(wmi_priv.po_data);
+}
diff --git a/drivers/platform/x86/dell-wmi-sysman/passwordattr-interface.c b/drivers/platform/x86/dell-wmi-sysman/passwordattr-interface.c
new file mode 100644
index 000000000000..40854a2c09a8
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/passwordattr-interface.c
@@ -0,0 +1,167 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to SET password methods under BIOS attributes interface GUID
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#include <linux/nls.h>
+#include <linux/wmi.h>
+#include "dell-wmi-sysman.h"
+
+static int call_password_interface(struct wmi_device *wdev, char *in_args, size_t size)
+{
+	struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_buffer input;
+	union acpi_object *obj;
+	acpi_status status;
+	int ret = -EIO;
+
+	input.length =  (acpi_size) size;
+	input.pointer = in_args;
+	status = wmidev_evaluate_method(wdev, 0, 1, &input, &output);
+	if (ACPI_FAILURE(status))
+		return -EIO;
+	obj = (union acpi_object *)output.pointer;
+	if (obj->type == ACPI_TYPE_INTEGER)
+		ret = obj->integer.value;
+
+	kfree(output.pointer);
+	/* let userland know it may need to check is_password_set again */
+	kobject_uevent(&wdev->dev.kobj, KOBJ_CHANGE);
+	return map_wmi_error(ret);
+}
+
+/**
+ * set_new_password() - Sets a system admin password
+ * @password_type: The type of password to set
+ * @new: The new password
+ *
+ * Sets the password using plaintext interface
+ **/
+int set_new_password(const char *password_type, const char *new)
+{
+	char *current_password, *type_value, *old_value, *new_value;
+	size_t security_area_size, string_area_size, buffer_size;
+	u8 *type_len, *old_len, *new_len;
+	char *buffer = NULL;
+	int ret;
+
+	mutex_lock(&wmi_priv.mutex);
+	if (!wmi_priv.password_attr_wdev) {
+		ret = -ENODEV;
+		goto out;
+	}
+	if (strcmp(password_type, "Admin") == 0) {
+		current_password = wmi_priv.current_admin_password;
+	} else if (strcmp(password_type, "System") == 0) {
+		current_password = wmi_priv.current_system_password;
+	} else {
+		ret = -EINVAL;
+		dev_err(&wmi_priv.password_attr_wdev->dev, "unknown password type %s\n",
+			password_type);
+		goto out;
+	}
+
+	/* build/calculate buffer */
+	security_area_size = calculate_security_buffer();
+	string_area_size = (strlen(password_type) + strlen(current_password) + strlen(new))*2;
+	buffer_size = security_area_size + string_area_size + sizeof(u16) * 3;
+	buffer = kzalloc(buffer_size, GFP_KERNEL);
+	if (!buffer) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/* build security area */
+	if (strlen(wmi_priv.current_admin_password) > 0)
+		populate_security_buffer(buffer, wmi_priv.current_admin_password);
+
+	/* build variables to set */
+	type_len = buffer + security_area_size;
+	type_value = type_len + sizeof(u16);
+	*type_len = utf8s_to_utf16s(password_type, strlen(password_type), UTF16_HOST_ENDIAN,
+				    (wchar_t *) type_value, MAX_BUFF) * 2;
+	if (*type_len < 0) {
+		ret = -EINVAL;
+		dev_err(&wmi_priv.password_attr_wdev->dev, "UTF16 conversion failed\n");
+		goto out;
+	}
+
+	old_len = type_value + *type_len;
+	old_value = old_len + sizeof(u16);
+	*old_len = utf8s_to_utf16s(current_password, strlen(current_password), UTF16_HOST_ENDIAN,
+				   (wchar_t *) old_value, MAX_BUFF) * 2;
+	if (*old_len < 0) {
+		ret = -EINVAL;
+		dev_err(&wmi_priv.password_attr_wdev->dev, "UTF16 conversion failed\n");
+		goto out;
+	}
+
+	new_len = old_value + *old_len;
+	new_value = new_len + sizeof(u16);
+	*new_len = utf8s_to_utf16s(new, strlen(new), UTF16_HOST_ENDIAN,
+				   (wchar_t *) new_value, MAX_BUFF) * 2;
+	if (*new_len < 0) {
+		ret = -EINVAL;
+		dev_err(&wmi_priv.password_attr_wdev->dev, "UTF16 conversion failed\n");
+		goto out;
+	}
+
+	ret = call_password_interface(wmi_priv.password_attr_wdev, buffer, buffer_size);
+	/* clear current_password here and use user input from wmi_priv.current_password */
+	if (!ret)
+		current_password = "";
+	/* explain to user the detailed failure reason */
+	else if (ret == -EOPNOTSUPP)
+		dev_err(&wmi_priv.password_attr_wdev->dev, "admin password must be configured\n");
+	else if (ret == -EACCES)
+		dev_err(&wmi_priv.password_attr_wdev->dev, "invalid password\n");
+
+out:
+	kfree(buffer);
+	mutex_unlock(&wmi_priv.mutex);
+
+	return ret;
+}
+
+static int bios_attr_pass_interface_probe(struct wmi_device *wdev, const void *context)
+{
+	mutex_lock(&wmi_priv.mutex);
+	wmi_priv.password_attr_wdev = wdev;
+	mutex_unlock(&wmi_priv.mutex);
+	return 0;
+}
+
+static int bios_attr_pass_interface_remove(struct wmi_device *wdev)
+{
+	mutex_lock(&wmi_priv.mutex);
+	wmi_priv.password_attr_wdev = NULL;
+	mutex_unlock(&wmi_priv.mutex);
+	return 0;
+}
+
+static const struct wmi_device_id bios_attr_pass_interface_id_table[] = {
+	{ .guid_string = DELL_WMI_BIOS_PASSWORD_INTERFACE_GUID },
+	{ },
+};
+static struct wmi_driver bios_attr_pass_interface_driver = {
+	.driver = {
+		.name = DRIVER_NAME"-password"
+	},
+	.probe = bios_attr_pass_interface_probe,
+	.remove = bios_attr_pass_interface_remove,
+	.id_table = bios_attr_pass_interface_id_table,
+};
+
+int init_bios_attr_pass_interface(void)
+{
+	return wmi_driver_register(&bios_attr_pass_interface_driver);
+}
+
+void exit_bios_attr_pass_interface(void)
+{
+	wmi_driver_unregister(&bios_attr_pass_interface_driver);
+}
+
+MODULE_DEVICE_TABLE(wmi, bios_attr_pass_interface_id_table);
diff --git a/drivers/platform/x86/dell-wmi-sysman/string-attributes.c b/drivers/platform/x86/dell-wmi-sysman/string-attributes.c
new file mode 100644
index 000000000000..6b9948157b8d
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/string-attributes.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to string type attributes under BIOS String GUID for use with
+ * dell-wmi-sysman
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#include "dell-wmi-sysman.h"
+
+enum string_properties {MIN_LEN = 6, MAX_LEN};
+
+get_instance_id(str);
+
+static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	int instance_id;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+	instance_id = get_str_instance_id(kobj);
+	if (instance_id >= 0) {
+		union acpi_object *obj;
+
+		/* need to use specific instance_id and guid combination to get right data */
+		obj = get_wmiobj_pointer(instance_id, DELL_WMI_BIOS_STRING_ATTRIBUTE_GUID);
+		if (!obj)
+			return -AE_ERROR;
+		strlcpy_attr(wmi_priv.str_data[instance_id].current_value,
+			obj->package.elements[CURRENT_VAL].string.pointer);
+		kfree(obj);
+		return sprintf(buf, "%s\n", wmi_priv.str_data[instance_id].current_value);
+	}
+	return -EIO;
+}
+
+/**
+ * validate_str_input() - Validate input of current_value against min and max lengths
+ * @instance_id: The instance on which input is validated
+ * @buf: Input value
+ **/
+static int validate_str_input(int instance_id, const char *buf)
+{
+	int in_len = strlen(buf);
+
+	if ((in_len >= wmi_priv.str_data[instance_id].min_length) &&
+	(in_len <= wmi_priv.str_data[instance_id].max_length))
+		return 0;
+
+	return -EINVAL;
+}
+
+attribute_s_property_show(display_name_language_code, str);
+static struct kobj_attribute str_displ_langcode =
+		__ATTR_RO(display_name_language_code);
+
+attribute_s_property_show(display_name, str);
+static struct kobj_attribute str_displ_name =
+		__ATTR_RO(display_name);
+
+attribute_s_property_show(default_value, str);
+static struct kobj_attribute str_default_val =
+		__ATTR_RO(default_value);
+
+attribute_property_store(current_value, str);
+static struct kobj_attribute str_current_val =
+		__ATTR_RW(current_value);
+
+attribute_s_property_show(dell_modifier, str);
+static struct kobj_attribute str_modifier =
+		__ATTR_RO(dell_modifier);
+
+attribute_n_property_show(min_length, str);
+static struct kobj_attribute str_min_length =
+		__ATTR_RO(min_length);
+
+attribute_n_property_show(max_length, str);
+static struct kobj_attribute str_max_length =
+		__ATTR_RO(max_length);
+
+attribute_s_property_show(type, str);
+static struct kobj_attribute str_type =
+	__ATTR_RO(type);
+
+static struct attribute *str_attrs[] = {
+	&str_displ_langcode.attr,
+	&str_displ_name.attr,
+	&str_default_val.attr,
+	&str_current_val.attr,
+	&str_modifier.attr,
+	&str_min_length.attr,
+	&str_max_length.attr,
+	&str_type.attr,
+	NULL,
+};
+
+static const struct attribute_group str_attr_group = {
+	.attrs = str_attrs,
+};
+
+int alloc_str_data(void)
+{
+	int ret = 0;
+
+	wmi_priv.str_instances_count = get_instance_count(DELL_WMI_BIOS_STRING_ATTRIBUTE_GUID);
+	wmi_priv.str_data = kcalloc(wmi_priv.str_instances_count,
+					sizeof(struct str_data), GFP_KERNEL);
+	if (!wmi_priv.str_data)
+		ret = -ENOMEM;
+	return ret;
+}
+
+/**
+ * populate_str_data() - Populate all properties of an instance under string attribute
+ * @str_obj: ACPI object with integer data
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ **/
+int populate_str_data(union acpi_object *str_obj, int instance_id, struct kobject *attr_name_kobj)
+{
+	wmi_priv.str_data[instance_id].attr_name_kobj = attr_name_kobj;
+	strlcpy_attr(wmi_priv.str_data[instance_id].attribute_name,
+		     str_obj[ATTR_NAME].string.pointer);
+	strlcpy_attr(wmi_priv.str_data[instance_id].display_name_language_code,
+		     str_obj[DISPL_NAME_LANG_CODE].string.pointer);
+	strlcpy_attr(wmi_priv.str_data[instance_id].display_name,
+		     str_obj[DISPLAY_NAME].string.pointer);
+	strlcpy_attr(wmi_priv.str_data[instance_id].default_value,
+		     str_obj[DEFAULT_VAL].string.pointer);
+	strlcpy_attr(wmi_priv.str_data[instance_id].current_value,
+		     str_obj[CURRENT_VAL].string.pointer);
+	strlcpy_attr(wmi_priv.str_data[instance_id].dell_modifier,
+		     str_obj[MODIFIER].string.pointer);
+	wmi_priv.str_data[instance_id].min_length = (uintptr_t)str_obj[MIN_LEN].string.pointer;
+	wmi_priv.str_data[instance_id].max_length = (uintptr_t) str_obj[MAX_LEN].string.pointer;
+	strlcpy_attr(wmi_priv.str_data[instance_id].type, "string");
+
+	return sysfs_create_group(attr_name_kobj, &str_attr_group);
+}
+
+/**
+ * exit_str_attributes() - Clear all attribute data
+ *
+ * Clears all data allocated for this group of attributes
+ **/
+void exit_str_attributes(void)
+{
+	int instance_id;
+
+	for (instance_id = 0; instance_id < wmi_priv.str_instances_count; instance_id++) {
+		if (wmi_priv.str_data[instance_id].attr_name_kobj)
+			sysfs_remove_group(wmi_priv.str_data[instance_id].attr_name_kobj,
+								&str_attr_group);
+	}
+	kfree(wmi_priv.str_data);
+}
diff --git a/drivers/platform/x86/dell-wmi-sysman/sysman.c b/drivers/platform/x86/dell-wmi-sysman/sysman.c
new file mode 100644
index 000000000000..df32abe5261e
--- /dev/null
+++ b/drivers/platform/x86/dell-wmi-sysman/sysman.c
@@ -0,0 +1,589 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Common methods for use with dell-wmi-sysman
+ *
+ *  Copyright (c) 2020 Dell Inc.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/fs.h>
+#include <linux/dmi.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/wmi.h>
+#include "dell-wmi-sysman.h"
+
+#define MAX_TYPES  4
+
+static struct class firmware_attributes_class = {
+	.name = "firmware-attributes",
+};
+
+struct wmi_sysman_priv wmi_priv = {
+	.mutex = __MUTEX_INITIALIZER(wmi_priv.mutex),
+};
+
+/* reset bios to defaults */
+static const char * const reset_types[] = {"builtinsafe", "lastknowngood", "factory", "custom"};
+static int reset_option = -1;
+
+/**
+ * calculate_security_buffer() - determines size of security buffer for authentication scheme
+ *
+ * Currently only supported type is Admin password
+ **/
+size_t calculate_security_buffer(void)
+{
+	if (strlen(wmi_priv.current_admin_password) > 0) {
+		return (sizeof(u32) * 2) +
+			strlen(wmi_priv.current_admin_password) +
+			strlen(wmi_priv.current_admin_password) % 2;
+	}
+	return sizeof(u32) * 2;
+}
+
+/**
+ * populate_security_buffer() - builds a security buffer for authentication scheme
+ * @buffer: the buffer to populate
+ * @authentication: the authentication content
+ *
+ * Currently only supported type is PLAIN TEXT
+ **/
+void populate_security_buffer(char *buffer, char *authentication)
+{
+	char *auth = buffer + sizeof(u32)*2;
+	u32 *sectype = (u32 *) buffer;
+	u32 *seclen = sectype + 1;
+	/* plain text */
+	*sectype = 1;
+	*seclen = strlen(authentication);
+	memcpy(auth, authentication, *seclen);
+}
+
+/**
+ * map_wmi_error() - map errors from WMI methods to kernel error codes
+ * @error_code: integer error code returned from Dell's firmware
+ **/
+int map_wmi_error(int error_code)
+{
+	switch (error_code) {
+	case 0:
+		/* success */
+		return 0;
+	case 1:
+		/* failed */
+		return -EIO;
+	case 2:
+		/* invalid parameter */
+		return -EINVAL;
+	case 3:
+		/* access denied */
+		return -EACCES;
+	case 4:
+		/* not supported */
+		return -EOPNOTSUPP;
+	case 5:
+		/* memory error */
+		return -ENOMEM;
+	case 6:
+		/* protocol error */
+		return -EPROTO;
+	}
+	/* unspecified error */
+	return -EIO;
+}
+
+/**
+ * reset_bios_show() - sysfs implementaton for read reset_bios
+ * @kobj: Kernel object for this attribute
+ * @attr: Kernel object attribute
+ * @buf: The buffer to display to userspace
+ **/
+static ssize_t reset_bios_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+	char *start = buf;
+	int i;
+
+	for (i = 0; i < MAX_TYPES; i++) {
+		if (i == reset_option)
+			buf += sprintf(buf, "[%s] ", reset_types[i]);
+		else
+			buf += sprintf(buf, "%s ", reset_types[i]);
+	}
+	buf += sprintf(buf, "\n");
+	return buf-start;
+}
+
+/**
+ * reset_bios_store() - sysfs implementaton for write reset_bios
+ * @kobj: Kernel object for this attribute
+ * @attr: Kernel object attribute
+ * @buf: The buffer from userspace
+ * @count: the size of the buffer from userspace
+ **/
+static ssize_t reset_bios_store(struct kobject *kobj,
+				struct kobj_attribute *attr, const char *buf, size_t count)
+{
+	int len, ret, i;
+	int type = -1;
+	char *p;
+
+	p = memchr(buf, '\n', count);
+	if (p != NULL)
+		*p = '\0';
+	len = p ? p - buf : count;
+
+	for (i = 0; i < MAX_TYPES; i++) {
+		if (len == strlen(reset_types[i])
+		    && !strncmp(buf, reset_types[i], len)) {
+			type = i;
+			break;
+		}
+	}
+
+	if (type < 0 || type >= MAX_TYPES) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	ret = set_bios_defaults(type);
+	pr_debug("reset all attributes request type %d: %d\n", type, ret);
+	if (ret) {
+		ret = -EINVAL;
+	} else {
+		reset_option = type;
+		ret = count;
+	}
+
+out:
+	return ret;
+}
+
+/**
+ * pending_reboot_show() - sysfs implementaton for read pending_reboot
+ * @kobj: Kernel object for this attribute
+ * @attr: Kernel object attribute
+ * @buf: The buffer to display to userspace
+ *
+ * Stores default value as 0
+ * When current_value is changed this attribute is set to 1 to notify reboot may be required
+ **/
+static ssize_t pending_reboot_show(struct kobject *kobj, struct kobj_attribute *attr,
+				   char *buf)
+{
+	return sprintf(buf, "%d\n", wmi_priv.pending_changes);
+}
+
+static struct kobj_attribute reset_bios = __ATTR_RW(reset_bios);
+static struct kobj_attribute pending_reboot = __ATTR_RO(pending_reboot);
+
+
+/**
+ * create_reset_bios() - Creates reset_bios and pending_reboot attributes
+ **/
+static int create_reset_bios(void)
+{
+	int ret = sysfs_create_file(&wmi_priv.main_dir_kset->kobj, &reset_bios.attr);
+
+	if (ret) {
+		pr_debug("could not create reset_bios file\n");
+		return ret;
+	}
+
+	ret = sysfs_create_file(&wmi_priv.main_dir_kset->kobj, &pending_reboot.attr);
+	if (ret) {
+		pr_debug("could not create changing_pending_reboot file\n");
+		sysfs_remove_file(&wmi_priv.main_dir_kset->kobj, &reset_bios.attr);
+	}
+	return ret;
+}
+
+static void release_reset_bios_data(void)
+{
+	sysfs_remove_file(&wmi_priv.main_dir_kset->kobj, &reset_bios.attr);
+	sysfs_remove_file(&wmi_priv.main_dir_kset->kobj, &pending_reboot.attr);
+}
+
+static ssize_t wmi_sysman_attr_show(struct kobject *kobj, struct attribute *attr,
+				    char *buf)
+{
+	struct kobj_attribute *kattr;
+	ssize_t ret = -EIO;
+
+	kattr = container_of(attr, struct kobj_attribute, attr);
+	if (kattr->show)
+		ret = kattr->show(kobj, kattr, buf);
+	return ret;
+}
+
+static ssize_t wmi_sysman_attr_store(struct kobject *kobj, struct attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct kobj_attribute *kattr;
+	ssize_t ret = -EIO;
+
+	kattr = container_of(attr, struct kobj_attribute, attr);
+	if (kattr->store)
+		ret = kattr->store(kobj, kattr, buf, count);
+	return ret;
+}
+
+const struct sysfs_ops wmi_sysman_kobj_sysfs_ops = {
+	.show	= wmi_sysman_attr_show,
+	.store	= wmi_sysman_attr_store,
+};
+
+static void attr_name_release(struct kobject *kobj)
+{
+	kfree(kobj);
+}
+
+static struct kobj_type attr_name_ktype = {
+	.release	= attr_name_release,
+	.sysfs_ops	= &wmi_sysman_kobj_sysfs_ops,
+};
+
+/**
+ * strlcpy_attr - Copy a length-limited, NULL-terminated string with bound checks
+ * @dest: Where to copy the string to
+ * @src: Where to copy the string from
+ **/
+void strlcpy_attr(char *dest, char *src)
+{
+	size_t len = strlen(src) + 1;
+
+	if (len > 1 && len < MAX_BUFF)
+		strlcpy(dest, src, len);
+
+	/*len can be zero because any property not-applicable to attribute can
+	 * be empty so check only for too long buffers and log error
+	 */
+	if (len > MAX_BUFF)
+		pr_err("Source string returned from BIOS is out of bound!\n");
+}
+
+/**
+ * get_wmiobj_pointer() - Get Content of WMI block for particular instance
+ * @instance_id: WMI instance ID
+ * @guid_string: WMI GUID (in str form)
+ *
+ * Fetches the content for WMI block (instance_id) under GUID (guid_string)
+ * Caller must kfree the return
+ **/
+union acpi_object *get_wmiobj_pointer(int instance_id, const char *guid_string)
+{
+	struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_status status;
+
+	status = wmi_query_block(guid_string, instance_id, &out);
+
+	return ACPI_SUCCESS(status) ? (union acpi_object *)out.pointer : NULL;
+}
+
+/**
+ * get_instance_count() - Compute total number of instances under guid_string
+ * @guid_string: WMI GUID (in string form)
+ **/
+int get_instance_count(const char *guid_string)
+{
+	union acpi_object *wmi_obj = NULL;
+	int i = 0;
+
+	do {
+		kfree(wmi_obj);
+		wmi_obj = get_wmiobj_pointer(i, guid_string);
+		i++;
+	} while (wmi_obj);
+
+	return (i-1);
+}
+
+/**
+ * alloc_attributes_data() - Allocate attributes data for a particular type
+ * @attr_type: Attribute type to allocate
+ **/
+static int alloc_attributes_data(int attr_type)
+{
+	int retval = 0;
+
+	switch (attr_type) {
+	case ENUM:
+		retval = alloc_enum_data();
+		break;
+	case INT:
+		retval = alloc_int_data();
+		break;
+	case STR:
+		retval = alloc_str_data();
+		break;
+	case PO:
+		retval = alloc_po_data();
+		break;
+	default:
+		break;
+	}
+
+	return retval;
+}
+
+/**
+ * destroy_attribute_objs() - Free a kset of kobjects
+ * @kset: The kset to destroy
+ *
+ * Fress kobjects created for each attribute_name under attribute type kset
+ **/
+static void destroy_attribute_objs(struct kset *kset)
+{
+	struct kobject *pos, *next;
+
+	list_for_each_entry_safe(pos, next, &kset->list, entry) {
+		kobject_put(pos);
+	}
+}
+
+/**
+ * release_attributes_data() - Clean-up all sysfs directories and files created
+ **/
+static void release_attributes_data(void)
+{
+	release_reset_bios_data();
+
+	mutex_lock(&wmi_priv.mutex);
+	exit_enum_attributes();
+	exit_int_attributes();
+	exit_str_attributes();
+	exit_po_attributes();
+	destroy_attribute_objs(wmi_priv.main_dir_kset);
+	destroy_attribute_objs(wmi_priv.authentication_dir_kset);
+	kset_unregister(wmi_priv.main_dir_kset);
+	kset_unregister(wmi_priv.authentication_dir_kset);
+	mutex_unlock(&wmi_priv.mutex);
+
+}
+
+/**
+ * init_bios_attributes() - Initialize all attributes for a type
+ * @attr_type: The attribute type to initialize
+ * @guid: The WMI GUID associated with this type to initialize
+ *
+ * Initialiaze all 4 types of attributes enumeration, integer, string and password object.
+ * Populates each attrbute typ's respective properties under sysfs files
+ **/
+static int init_bios_attributes(int attr_type, const char *guid)
+{
+	struct kobject *attr_name_kobj; //individual attribute names
+	union acpi_object *obj = NULL;
+	union acpi_object *elements;
+	struct kset *tmp_set;
+
+	/* instance_id needs to be reset for each type GUID
+	 * also, instance IDs are unique within GUID but not across
+	 */
+	int instance_id = 0;
+	int retval = 0;
+
+	retval = alloc_attributes_data(attr_type);
+	if (retval)
+		return retval;
+	/* need to use specific instance_id and guid combination to get right data */
+	obj = get_wmiobj_pointer(instance_id, guid);
+	if (!obj)
+		return -ENODEV;
+	elements = obj->package.elements;
+
+	mutex_lock(&wmi_priv.mutex);
+	while (elements) {
+		/* sanity checking */
+		if (strlen(elements[ATTR_NAME].string.pointer) == 0) {
+			pr_debug("empty attribute found\n");
+			goto nextobj;
+		}
+		if (attr_type == PO)
+			tmp_set = wmi_priv.authentication_dir_kset;
+		else
+			tmp_set = wmi_priv.main_dir_kset;
+
+		if (kset_find_obj(tmp_set, elements[ATTR_NAME].string.pointer)) {
+			pr_debug("duplicate attribute name found - %s\n",
+				elements[ATTR_NAME].string.pointer);
+			goto nextobj;
+		}
+
+		/* build attribute */
+		attr_name_kobj = kzalloc(sizeof(*attr_name_kobj), GFP_KERNEL);
+		if (!attr_name_kobj)
+			goto err_attr_init;
+
+		attr_name_kobj->kset = tmp_set;
+
+		retval = kobject_init_and_add(attr_name_kobj, &attr_name_ktype, NULL, "%s",
+						elements[ATTR_NAME].string.pointer);
+		if (retval) {
+			kobject_put(attr_name_kobj);
+			goto err_attr_init;
+		}
+
+		/* enumerate all of this attribute */
+		switch (attr_type) {
+		case ENUM:
+			retval = populate_enum_data(elements, instance_id, attr_name_kobj);
+			break;
+		case INT:
+			retval = populate_int_data(elements, instance_id, attr_name_kobj);
+			break;
+		case STR:
+			retval = populate_str_data(elements, instance_id, attr_name_kobj);
+			break;
+		case PO:
+			retval = populate_po_data(elements, instance_id, attr_name_kobj);
+			break;
+		default:
+			break;
+		}
+
+		if (retval) {
+			pr_debug("failed to populate %s\n",
+				elements[ATTR_NAME].string.pointer);
+			goto err_attr_init;
+		}
+
+nextobj:
+		kfree(obj);
+		instance_id++;
+		obj = get_wmiobj_pointer(instance_id, guid);
+		elements = obj ? obj->package.elements : NULL;
+	}
+
+	goto out;
+
+err_attr_init:
+	release_attributes_data();
+	kfree(obj);
+out:
+	mutex_unlock(&wmi_priv.mutex);
+	return retval;
+}
+
+static int __init sysman_init(void)
+{
+	int ret = 0;
+	struct device *class_dev;
+
+	if (!dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "Dell System", NULL) &&
+	    !dmi_find_device(DMI_DEV_TYPE_OEM_STRING, "www.dell.com", NULL)) {
+		pr_err("Unable to run on non-Dell system\n");
+		return -ENODEV;
+	}
+
+	ret = init_bios_attr_set_interface();
+	if (ret || !wmi_priv.bios_attr_wdev) {
+		pr_debug("failed to initialize set interface\n");
+		goto fail_set_interface;
+	}
+
+	ret = init_bios_attr_pass_interface();
+	if (ret || !wmi_priv.password_attr_wdev) {
+		pr_debug("failed to initialize pass interface\n");
+		goto fail_pass_interface;
+	}
+
+	ret = class_register(&firmware_attributes_class);
+	if (ret)
+		goto fail_class;
+
+	class_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
+				  NULL, "%s", DRIVER_NAME);
+	if (IS_ERR(class_dev)) {
+		ret = PTR_ERR(class_dev);
+		goto fail_classdev;
+	}
+
+	wmi_priv.main_dir_kset = kset_create_and_add("attributes", NULL,
+						     &class_dev->kobj);
+	if (!wmi_priv.main_dir_kset) {
+		ret = -ENOMEM;
+		goto fail_main_kset;
+	}
+
+	wmi_priv.authentication_dir_kset = kset_create_and_add("authentication", NULL,
+								&class_dev->kobj);
+	if (!wmi_priv.authentication_dir_kset) {
+		ret = -ENOMEM;
+		goto fail_authentication_kset;
+	}
+
+	ret = create_reset_bios();
+	if (ret) {
+		pr_debug("could not create reset BIOS attribute\n");
+		goto fail_reset_bios;
+	}
+
+	ret = init_bios_attributes(ENUM, DELL_WMI_BIOS_ENUMERATION_ATTRIBUTE_GUID);
+	if (ret) {
+		pr_debug("failed to populate enumeration type attributes\n");
+		goto fail_create_group;
+	}
+
+	ret = init_bios_attributes(INT, DELL_WMI_BIOS_INTEGER_ATTRIBUTE_GUID);
+	if (ret) {
+		pr_debug("failed to populate integer type attributes\n");
+		goto fail_create_group;
+	}
+
+	ret = init_bios_attributes(STR, DELL_WMI_BIOS_STRING_ATTRIBUTE_GUID);
+	if (ret) {
+		pr_debug("failed to populate string type attributes\n");
+		goto fail_create_group;
+	}
+
+	ret = init_bios_attributes(PO, DELL_WMI_BIOS_PASSOBJ_ATTRIBUTE_GUID);
+	if (ret) {
+		pr_debug("failed to populate pass object type attributes\n");
+		goto fail_create_group;
+	}
+
+	return 0;
+
+fail_create_group:
+	release_attributes_data();
+
+fail_reset_bios:
+	kset_unregister(wmi_priv.authentication_dir_kset);
+
+fail_authentication_kset:
+	kset_unregister(wmi_priv.main_dir_kset);
+
+fail_main_kset:
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
+
+fail_classdev:
+	class_unregister(&firmware_attributes_class);
+
+fail_class:
+	exit_bios_attr_pass_interface();
+
+fail_pass_interface:
+	exit_bios_attr_set_interface();
+
+fail_set_interface:
+	return ret;
+}
+
+static void __exit sysman_exit(void)
+{
+	release_attributes_data();
+	exit_bios_attr_set_interface();
+	exit_bios_attr_pass_interface();
+	device_destroy(&firmware_attributes_class, MKDEV(0, 0));
+	class_unregister(&firmware_attributes_class);
+}
+
+module_init(sysman_init);
+module_exit(sysman_exit);
+
+MODULE_AUTHOR("Mario Limonciello <mario.limonciello@dell.com>");
+MODULE_AUTHOR("Prasanth Ksr <prasanth.ksr@dell.com>");
+MODULE_AUTHOR("Divya Bharathi <divya.bharathi@dell.com>");
+MODULE_DESCRIPTION("Dell platform setting control interface");
+MODULE_LICENSE("GPL");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-23 11:30 [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems Divya Bharathi
@ 2020-09-23 17:11 ` Randy Dunlap
  2020-09-23 18:39   ` Limonciello, Mario
  2020-09-25 13:26 ` Hans de Goede
  1 sibling, 1 reply; 9+ messages in thread
From: Randy Dunlap @ 2020-09-23 17:11 UTC (permalink / raw)
  To: Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Divya Bharathi, Hans de Goede,
	Andy Shevchenko, mark gross, Mario Limonciello, Prasanth KSR

Hi,

On 9/23/20 4:30 AM, Divya Bharathi wrote:
> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> index 40219bba6801..4fd7a3f0a904 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -430,6 +430,18 @@ config DELL_WMI
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called dell-wmi.
>  
> +config DELL_WMI_SYSMAN
> +	tristate "Dell WMI Systems management WMI driver"

Could you drop one of those "WMI" strings or are both of them needed?

> +	depends on ACPI_WMI
> +	depends on DMI
> +	select NLS
> +	help
> +	  This driver allows changing BIOS settings on many Dell machines from
> +	  2018 and newer without the use of any additional software.
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called dell-wmi-sysman.

thanks.
-- 
~Randy


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-23 17:11 ` Randy Dunlap
@ 2020-09-23 18:39   ` Limonciello, Mario
  2020-09-23 18:41     ` Randy Dunlap
  0 siblings, 1 reply; 9+ messages in thread
From: Limonciello, Mario @ 2020-09-23 18:39 UTC (permalink / raw)
  To: Randy Dunlap, Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Bharathi, Divya, Hans de Goede,
	Andy Shevchenko, mark gross, Ksr, Prasanth

> -----Original Message-----
> From: Randy Dunlap <rdunlap@infradead.org>
> Sent: Wednesday, September 23, 2020 12:12
> To: Divya Bharathi; dvhart@infradead.org
> Cc: LKML; platform-driver-x86@vger.kernel.org; Bharathi, Divya; Hans de Goede;
> Andy Shevchenko; mark gross; Limonciello, Mario; Ksr, Prasanth
> Subject: Re: [PATCH v4] Introduce support for Systems Management Driver over
> WMI for Dell Systems
> 
> 
> [EXTERNAL EMAIL]
> 
> Hi,
> 
> On 9/23/20 4:30 AM, Divya Bharathi wrote:
> > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
> > index 40219bba6801..4fd7a3f0a904 100644
> > --- a/drivers/platform/x86/Kconfig
> > +++ b/drivers/platform/x86/Kconfig
> > @@ -430,6 +430,18 @@ config DELL_WMI
> >  	  To compile this driver as a module, choose M here: the module will
> >  	  be called dell-wmi.
> >
> > +config DELL_WMI_SYSMAN
> > +	tristate "Dell WMI Systems management WMI driver"
> 
> Could you drop one of those "WMI" strings or are both of them needed?
> 

Thanks, that's a good suggestion.  How about instead:

"Dell WMI based Systems management driver"

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-23 18:39   ` Limonciello, Mario
@ 2020-09-23 18:41     ` Randy Dunlap
  0 siblings, 0 replies; 9+ messages in thread
From: Randy Dunlap @ 2020-09-23 18:41 UTC (permalink / raw)
  To: Limonciello, Mario, Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Bharathi, Divya, Hans de Goede,
	Andy Shevchenko, mark gross, Ksr, Prasanth

On 9/23/20 11:39 AM, Limonciello, Mario wrote:
>> -----Original Message-----
>> From: Randy Dunlap <rdunlap@infradead.org>
>> Sent: Wednesday, September 23, 2020 12:12
>> To: Divya Bharathi; dvhart@infradead.org
>> Cc: LKML; platform-driver-x86@vger.kernel.org; Bharathi, Divya; Hans de Goede;
>> Andy Shevchenko; mark gross; Limonciello, Mario; Ksr, Prasanth
>> Subject: Re: [PATCH v4] Introduce support for Systems Management Driver over
>> WMI for Dell Systems
>>
>>
>> [EXTERNAL EMAIL]
>>
>> Hi,
>>
>> On 9/23/20 4:30 AM, Divya Bharathi wrote:
>>> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
>>> index 40219bba6801..4fd7a3f0a904 100644
>>> --- a/drivers/platform/x86/Kconfig
>>> +++ b/drivers/platform/x86/Kconfig
>>> @@ -430,6 +430,18 @@ config DELL_WMI
>>>  	  To compile this driver as a module, choose M here: the module will
>>>  	  be called dell-wmi.
>>>
>>> +config DELL_WMI_SYSMAN
>>> +	tristate "Dell WMI Systems management WMI driver"
>>
>> Could you drop one of those "WMI" strings or are both of them needed?
>>
> 
> Thanks, that's a good suggestion.  How about instead:
> 
> "Dell WMI based Systems management driver"

        WMI-based

OK. Thanks.

-- 
~Randy


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-23 11:30 [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems Divya Bharathi
  2020-09-23 17:11 ` Randy Dunlap
@ 2020-09-25 13:26 ` Hans de Goede
  2020-09-25 15:32   ` Limonciello, Mario
  1 sibling, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2020-09-25 13:26 UTC (permalink / raw)
  To: Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Divya Bharathi, Andy Shevchenko,
	mark gross, Mario Limonciello, Prasanth KSR

Hi,

So as usual I will split my remarks in 2 parts,
this first email will focus on the sysfs bits.

I will do a detailed review of the code coming Monday.

On 9/23/20 1:30 PM, Divya Bharathi wrote:
> The Dell WMI Systems Management Driver provides a sysfs
> interface for systems management to enable BIOS configuration
> capability on certain Dell Systems.
> 
> This driver allows user to configure Dell systems with a
> uniform common interface. To facilitate this, the patch
> introduces a generic way for driver to be able to create
> configurable BIOS Attributes available in Setup (F2) screen.
> 
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: mark gross <mgross@linux.intel.com>
> 
> Co-developed-by: Mario Limonciello <mario.limonciello@dell.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Co-developed-by: Prasanth KSR <prasanth.ksr@dell.com>
> Signed-off-by: Prasanth KSR <prasanth.ksr@dell.com>
> Signed-off-by: Divya Bharathi <divya.bharathi@dell.com>
> ---
> 
> Changes from v3 to v4:
>   - Create a firmware-attributes class and tie ksets to a virtual device in it
>   - Make modifier and value_modifier "dell only" attributes.
>   - Correct some errors caught by kernel build bot around missing prototypes
>   - Remove mutexes from populate_* functions and put in init_dell_bios_attrib_wmi instead
>   - Move all code into a subdirectory drivers/platform/x86/dell-wmi-sysman and remove dell-wmi-*
>     prefix on files
>   - Move all data structures into shared struct
>   - In alloc functions instead of kzalloc use kcalloc and check that there is no overflow
>     + Same check for other alloc_foo-data functions
>   -  populate_*: Move sysfs_create_group to end of the function to prevent race conditions
>   - Save kernel object into each data instance and only remove that rather than sysfs_remove_group
>   - Document in sysfs file what causes change uevents to come through
>   - Only notify with change uevent one time on multiple settings modifications
>   - Adjust lots of string handling
>   - Make more objects static
>   - Various whitespace corrections
>   - Document map_wmi_error properly
>   - Bump version to 5.11 (February 2021)

Thank you for all these changes.

Things are starting to look pretty good now to me, good work.


> 
> Changes from v2 to v3:
>   - Fix a possible NULL pointer error in init
>   - Add missing newlines to all dev_err/dev_dbg/pr_err/pr_debug statements
>   - Correct updating passwords when both Admin and System password are set
>   - Correct the WMI driver name
>   - Correct some namespace clashing when compiled into the kernel (Reported by Mark Gross)
>   - Correct some comment typos
>   - Adopt suggestions made by Hans:
>     + Use single global mutex
>     + Clarify reason for uevents with a comment
>     + Remove functions for set and get current password
>     + Rename lower_bound to min_value and upper_bound to max_value
>     + Rename possible_value to possible_values
>     + Remove references to float
>     + Build a separate passwords directory again since it behaves differently from the other
>       attributes
>     + Move more calls from pr_err -> dev_err
>   - Documentation cleanups (see v2 patch feedback)
>     + Grouping types
>     + Syntax of `modifier` output
> 
> Changes from v1 to v2:
>   - use pr_fmt instead of pr_err(DRIVER_NAME
>   - re-order variables reverse xmas tree order
>   - correct returns of -1 to error codes
>   - correct usage of {} on some split line statements
>   - Refine all documentation deficiencies suggested by Hans
>   - Merge all attributes to a single directory
>   - Overhaul WMI interface interaction as suggested by Hans
>     * Move WMI driver registration to start of module
>     * Remove usage of lists that only use first entry for WMI interfaces
>     * Create a global structure shared across interface source files
>     * Make get_current_password function static
>     * Remove get_pending changes function, shared across global structure now.
> - Overhaul use of mutexes
>     * Make kset list mutex shared across source files
>     * Remove unneeded dell-wmi-sysman call_mutex
>     * Keep remaining call_mutexes in WMI functions
> - Move security area calculation into a function
> - Use NLS helper for utf8->utf16 conversion
> 
>   .../testing/sysfs-class-firmware-attributes   | 199 ++++++
>   MAINTAINERS                                   |   8 +
>   drivers/platform/x86/Kconfig                  |  12 +
>   drivers/platform/x86/Makefile                 |   1 +
>   drivers/platform/x86/dell-wmi-sysman/Makefile |   8 +
>   .../x86/dell-wmi-sysman/biosattr-interface.c  | 199 ++++++
>   .../x86/dell-wmi-sysman/dell-wmi-sysman.h     | 196 ++++++
>   .../x86/dell-wmi-sysman/enum-attributes.c     | 188 ++++++
>   .../x86/dell-wmi-sysman/int-attributes.c      | 169 +++++
>   .../x86/dell-wmi-sysman/passobj-attributes.c  | 153 +++++
>   .../dell-wmi-sysman/passwordattr-interface.c  | 167 +++++
>   .../x86/dell-wmi-sysman/string-attributes.c   | 156 +++++
>   drivers/platform/x86/dell-wmi-sysman/sysman.c | 589 ++++++++++++++++++
>   13 files changed, 2045 insertions(+)
>   create mode 100644 Documentation/ABI/testing/sysfs-class-firmware-attributes
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/Makefile
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/biosattr-interface.c
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/dell-wmi-sysman.h
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/enum-attributes.c
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/int-attributes.c
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/passobj-attributes.c
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/passwordattr-interface.c
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/string-attributes.c
>   create mode 100644 drivers/platform/x86/dell-wmi-sysman/sysman.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-firmware-attributes b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> new file mode 100644
> index 000000000000..ad45d5717071
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-firmware-attributes
> @@ -0,0 +1,199 @@
> +What:		/sys/class/firmware-attributes/*/attributes/*/
> +Date:		February 2021
> +KernelVersion:	5.11
> +Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
> +		Mario Limonciello <mario.limonciello@dell.com>,
> +		Prasanth KSR <prasanth.ksr@dell.com>
> +Description:
> +		A sysfs interface for systems management software to enable
> +		configuration capability on supported systems.  This directory
> +		exposes interfaces for interacting with configuration options.
> +
> +		Attributes can accept:
> +		- a set of pre-defined valid values (enumeration)
> +		- a range of numerical values (integer)
> +		- a string
> +
> +		All attribute types support the following values:

This list seems to miss a "type" sysfs attribute, which tells which type
the firmware-attribute in question is. Sorry for not noticing that sooner.

> +
> +		current_value:	A file that can be read to obtain the current
> +		value of the <attr>

Can you indent the continuation on the second line to align with the start
of the description on the first line please?, e.g.:

		current_value:	A file that can be read to obtain the current
				value of the <attr>

This goes for all the sysfs-attribute description texts.

> +
> +		This file can also be written to in order to update
> +		the value of a <attr>
> +
> +		default_value:	A file that can be read to obtain the default
> +		value of the <attr>
> +
> +		display_name:	A file that can be read to obtain a user friendly
> +		description of the at <attr>
> +
> +		display_name_language_code:	A file that can be read to obtain
> +		the IETF language tag corresponding to the "display_name" of the <attr>
> +
> +		"enumeration"-type specific properties:
> +
> +		possible_values: A file that can be read to obtain the possible
> +		values of the <attr>. Values are separated using semi-colon.

As non-native English speaker I had to lookup semi-colon to make sure that
it indeed is ';' as I already sorta expected. So can we add "(';')" (without
the "") behind the semi-colon to make this easier to parse for non-native English
speakers?

> +		"integer"-type specific properties:
> +
> +		min_value:	A file that can be read to obtain the lower
> +		bound value of the <attr>
> +
> +		max_value:	A file that can be read to obtain the upper
> +		bound value of the <attr>
> +
> +		scalar_increment:	A file that can be read to obtain the
> +		resolution of the incremental value this attribute accepts.

Can we have an example here? I guess if for some reason only even/odd values
are allowed then this would contain "2" ?

> +
> +		"string"-type specific properties:
> +
> +		max_length:	A file that can be read to obtain the maximum
> +		length value of the <attr>
> +
> +		min_length:	A file that can be read to obtain the minimum
> +		length value of the <attr>

So I have been taking a look at a community written driven to allow changing
BIOS-settings / firmware-attributes on some Lenovo laptops:

https://github.com/iksaif/thinkpad-wmi

My main reason for doing so is to check if other vendors also support things
like "display_name", "default_value", etc.

So for the normal attributes, it seems that for the Thinkpad WMI interface they
are all enums and the do contain possible_values. So that is fine.

But they do not have a separate display_name only a name-name, nor do they
have a default_value.

So I think we should mark the display_name, display_name_language_code and
default_value sysfs-attributes optional, e.g. make the description look like this:

		default_value:	An optional file that can be read to obtain the
				default value of the <attr>



> +		Dell specific class extensions
> +		--------------------------
> +
> +		On Dell systems the following additional attributes are available:
> +
> +		dell_modifier: A file that can be read to obtain attribute-level
> +		dependency rule. It says an attribute X will become read-only or
> +		suppressed, if/if-not attribute Y is configured.
> +
> +		modifier rules can be in following format,
> +		[ReadOnlyIf:<attribute>=<value>]
> +		[ReadOnlyIfNot:<attribute>=<value>]
> +		[SuppressIf:<attribute>=<value>]
> +		[SuppressIfNot:<attribute>=<value>]
> +
> +		For example:
> +		AutoOnFri/dell_modifier has value,
> +			[SuppressIfNot:AutoOn=SelectDays]
> +		This means AutoOnFri will be suppressed in BIOS setup if AutoOn
> +		attribute is not "SelectDays" and its value will not be effective
> +		through sysfs until this rule is met.
> +
> +		Enumeration attributes also support the following:
> +
> +		dell_value_modifier:	A file that can be read to obtain value-level
> +		dependency. This file is similar to dell_modifier but here, an
> +		attribute's current value will be forcefully changed based dependent
> +		attributes value.
> +
> +		dell_value_modifier rules can be in following format,
> +		<value>[ForceIf:<attribute>=<value>]
> +		<value>[ForceIfNot:<attribute>=<value>]
> +
> +		For example,
> +		LegacyOrom/dell_value_modifier has value,
> +			Disabled[ForceIf:SecureBoot=Enabled]
> +		This means LegacyOrom's current value will be forced to "Disabled"
> +		in BIOS setup if SecureBoot is Enabled and its value will not be
> +		effective through sysfs until this rule is met.

Thank you for making these Dell specific.

> +What:		/sys/class/firmware-attributes/*/authentication/
> +Date:		February 2021
> +KernelVersion:	5.11
> +Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
> +		Mario Limonciello <mario.limonciello@dell.com>,
> +		Prasanth KSR <prasanth.ksr@dell.com>
> +
> +		Devices support various authentication mechanisms which can be exposed
> +		as a separate configuration object.
> +
> +		For example a "BIOS Admin" password and "System" Password can be set,
> +		reset or cleared using these attributes.
> +		- An "Admin" password is used for preventing modification to the BIOS
> +		  settings.
> +		- A "System" password is required to boot a machine.
> +
> +		Change in any of these two authentication methods will also generate an
> +		uevent KOBJ_CHANGE.
> +
> +		is_authentication_set:	A file that can be read
> +		to obtain flag to see if a password is set on <attr>
> +
> +		max_password_length:	A file that can be read to obtain the
> +		maximum length of the Password
> +
> +		min_password_length:	A file that can be read to obtain the
> +		minimum length of the Password
> +
> +		current_password: A write only value used for privileged access
> +		such as setting attributes when a system or admin password is set
> +		or resetting to a new password
> +
> +		new_password: A write only value that when used in tandem with
> +		current_password will reset a system or admin password.
> +
> +		Note, password management is session specific. If Admin password is set,
> +		same password must be written into current_password file (required for
> +		password-validation) and must be cleared once the session is over.
> +		For example:
> +			echo "password" > current_password
> +			echo "disabled" > TouchScreen/current_value
> +			echo "" > current_password
> +
> +		Drivers may emit a CHANGE uevent when a password is set or unset
> +		userspace may check it again.

First of all some generic remarks:

Currently the "Admin" and "System" names come directly from the Dell WMI
interface. I have 2 concerns with this:

1) What if we do get multiple authentication mechanisms for a single user,
e.g. both a type == "pasword" and type == "hotp" authentication. The way I have
been thinking about that sofar, is that we then get 2 admin dirs under the
/sys/class/firmware-attributes/*/authentication dir, with a type attribute
per dir, following how we do the attributes. So we would get e.g. these 2 dirs:

/sys/class/firmware-attributes/dell/authentication/admin-password
/sys/class/firmware-attributes/dell/authentication/admin-hotp

For the admin user. If want to do it like this in the future we should
add some indirection between the WMI username and the dir which is created
now and create the Admin dir as admin-password starting now.

2) The "Admin" name is clear enough, but the "System" name is somewhat
ambiguous and other vendors may call this differently, I think I have
at least seen it called the "User" password in some cases and Lenovo
seems to call it a power-on-password. I think that just calling it the
"boot" password makes sense. My main concern is that "System" is a bit
too vague. So then for now we would get:

/sys/class/firmware-attributes/dell/authentication/admin-password
/sys/class/firmware-attributes/dell/authentication/boot-password

The spec. should also specify that the part before the first '-' is the
username, and the part after it is the authentication type. E.g. the
docs for this could look something like this:

	Directories under /sys/class/firmware-attributes/*/authentication/
	use the following directory-name pattern:
	<username>-<authentication_method>

	Where username must be one of: "admin" or "boot":

	admin	If any authentication_method is enabled for the admin user, then
		authentication as the admin user is required to modify BIOS settings.
	boot	If any authentication_method is enabled for the admin user, then
		authentication as the boot user is required to boot the machine.

	And where authentication_method must be "password". Note in the future
	both more usernames and more authentication_method-s may be added.

	All authentication_methods must have the following sysfs-attributes:

	is_enabled:  This reads "1" if the authentication_method is enabled,
		     and "0" if its disabled

	Any changes to authentication_methods will generate a change uevent,
	upon receiving this event applications should recheck the authentication
	settings such as the is_enabled flag.

	Password authentication_method specific sysfs-attributes:

	max_password_length: ... (continue with the old text)

Note:

1) This is a proposal to make the authentication bits a bit more generic /
    future proof. This is very much open for discussion.

2) The new generic is_enabled sysfs-attribute replaces the is_authentication_set flag

###

So as with the actual firmware-attributes I have also been comparing the authentication
bits for the Dell case with the community thinkpad_wmi code. And again things are a pretty
good match already, including being able to query a minimum and maximin password length.

The only thing which is different, which I think would be good to add now, is
a password_encoding sysfs-attribute. The Lenovo password interface supports
2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
works on modern UEFI based platforms.

Since the Dell password code uses UTF8 to UTF16 translation routines, I guess
the encoding for the Dell password is UTF8 (at the sysfs level). So I would like to propose
an extra password-authentication attribute like:

	password_encoding:  A file that can be read to obtain the encoding used by
			    the current_password and new_password attributes.
			    The value returned should be one of: "utf8", "ascii".
			    In some cases this may return a vendor-specific encoding
			    like, e.g. "lenovo-scancodes".

And for the Dell driver this would just be hardcoded to utf8.

> +
> +		On Dell systems, if Admin password is set, then all BIOS attributes
> +		require password validation.

I think that this bit can be dropped both the new authentication text which I suggest
above, but even the old text:

> +		- An "Admin" password is used for preventing modification to the BIOS
> +		  settings.

Already makes it clear that if an Admin password is set modification of (any)
BIOS settings is not allowed without the password. So this seems redundant.

If in the future some attributes can be changed without the Admin password,
then we can deal with that then by introducing a new authorization_required
attribute, which could contain, e.g. one of "none, "admin", "<new-to-be-defined
-power-user-level-user>".

And when we define that optional attribute then we add that the absence of
said attribute means that the consumer of the API should behave as if the
authorization_requires attribute contains "admin", which existing API consumers
will already do as that is the behavior we define currently.


> +
> +What:		/sys/class/firmware-attributes/*/attributes/pending_reboot
> +Date:		February 2021
> +KernelVersion:	5.11
> +Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
> +		Mario Limonciello <mario.limonciello@dell.com>,
> +		Prasanth KSR <prasanth.ksr@dell.com>
> +Description:
> +		A read-only attribute reads 1 if a reboot is necessary to apply
> +		pending BIOS attribute changes. Also, an uevent_KOBJ_CHANGE is
> +		generated when it changes to 1.
> +
> +			0:	All BIOS attributes setting are current
> +			1:	A reboot is necessary to get pending BIOS attribute changes
> +				applied
> +
> +		Note, userspace applications need to follow below steps for efficient
> +		BIOS management,
> +		1.	Check if admin password is set. If yes, follow session method for
> +			password management as briefed under authentication section above.
> +		2.	Before setting any attribute, check if it has any modifiers
> +			or value_modifiers. If yes, incorporate them and then modify
> +			attribute.
> +
> +		Drivers may emit a CHANGE uevent when this value changes and userspace
> +		may check it again.
> +
> +What:		/sys/class/firmware-attributes/*/attributes/reset_bios
> +Date:		February 2021
> +KernelVersion:	5.11
> +Contact:	Divya Bharathi <Divya.Bharathi@Dell.com>,
> +		Mario Limonciello <mario.limonciello@dell.com>,
> +		Prasanth KSR <prasanth.ksr@dell.com>
> +Description:
> +		This attribute can be used to reset the BIOS Configuration.
> +		Specifically, it tells which type of reset BIOS configuration is being
> +		requested on the host.
> +
> +		Reading from it returns a list of supported options encoded as:
> +
> +			'builtinsafe' (Built in safe configuration profile)
> +			'lastknowngood' (Last known good saved configuration profile)
> +			'factory' (Default factory settings configuration profile)
> +			'custom' (Custom saved configuration profile)
> +
> +		The currently selected option is printed in square brackets as
> +		shown below:
> +
> +		# echo "factory" > /sys/class/firmware-attributes/*/device/attributes/reset_bios
> +		# cat /sys/class/firmware-attributes/*/device/attributes/reset_bios
> +		# builtinsafe lastknowngood [factory] custom
> +
> +		Note that any changes to this attribute requires a reboot
> +		for changes to take effect.

Regards,

Hans


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-25 13:26 ` Hans de Goede
@ 2020-09-25 15:32   ` Limonciello, Mario
  2020-09-25 18:16     ` Hans de Goede
  0 siblings, 1 reply; 9+ messages in thread
From: Limonciello, Mario @ 2020-09-25 15:32 UTC (permalink / raw)
  To: Hans de Goede, Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Bharathi, Divya, Andy Shevchenko,
	mark gross, Ksr, Prasanth

So I do want to preface this response by mentioning that Dell's implementation
is based off the PLDM specification from the DMTF.
https://www.dmtf.org/sites/default/files/standards/documents/DSP0247_1.0.0.pdf

A lot of the nomenclature that has been already proposed to change followed
nomenclature previously used in the PLDM specification.  In the spirit of matching
that specification in the Linux implementation we may consider to change things
like proposed to be renamed min_value back to lower_bound, but that's up to your
decision.

> 
> This list seems to miss a "type" sysfs attribute, which tells which type
> the firmware-attribute in question is. Sorry for not noticing that sooner.

Whoops :)

> 
> > +
> > +		current_value:	A file that can be read to obtain the current
> > +		value of the <attr>
> 
> Can you indent the continuation on the second line to align with the start
> of the description on the first line please?, e.g.:
> 
> 		current_value:	A file that can be read to obtain the current
> 				value of the <attr>
> 
> This goes for all the sysfs-attribute description texts.
> 
> > +
> > +		This file can also be written to in order to update
> > +		the value of a <attr>
> > +
> > +		default_value:	A file that can be read to obtain the default
> > +		value of the <attr>
> > +
> > +		display_name:	A file that can be read to obtain a user friendly
> > +		description of the at <attr>
> > +
> > +		display_name_language_code:	A file that can be read to obtain
> > +		the IETF language tag corresponding to the "display_name" of the
> <attr>
> > +
> > +		"enumeration"-type specific properties:
> > +
> > +		possible_values: A file that can be read to obtain the possible
> > +		values of the <attr>. Values are separated using semi-colon.
> 
> As non-native English speaker I had to lookup semi-colon to make sure that
> it indeed is ';' as I already sorta expected. So can we add "(';')" (without
> the "") behind the semi-colon to make this easier to parse for non-native
> English
> speakers?


Some parts of the kernel documentation seem to use semi-colon (``;``) - example of
admin-guide/bootconfig.rst

and others just set semi-colon - example of admin-guide/device-mapper/dm-init.rst.

But I don't see a problem with this change.

> 
> > +		"integer"-type specific properties:
> > +
> > +		min_value:	A file that can be read to obtain the lower
> > +		bound value of the <attr>
> > +
> > +		max_value:	A file that can be read to obtain the upper
> > +		bound value of the <attr>
> > +
> > +		scalar_increment:	A file that can be read to obtain the
> > +		resolution of the incremental value this attribute accepts.
> 
> Can we have an example here? I guess if for some reason only even/odd values
> are allowed then this would contain "2" ?

Maybe adopting the PLDM description is better to explain it.

"The scalar value that is used for the increments to this integer"
> 
> > +
> > +		"string"-type specific properties:
> > +
> > +		max_length:	A file that can be read to obtain the maximum
> > +		length value of the <attr>
> > +
> > +		min_length:	A file that can be read to obtain the minimum
> > +		length value of the <attr>
> 
> So I have been taking a look at a community written driven to allow changing
> BIOS-settings / firmware-attributes on some Lenovo laptops:
> 
> https://github.com/iksaif/thinkpad-wmi
> 
> My main reason for doing so is to check if other vendors also support things
> like "display_name", "default_value", etc.
> 
> So for the normal attributes, it seems that for the Thinkpad WMI interface
> they
> are all enums and the do contain possible_values. So that is fine.
> 
> But they do not have a separate display_name only a name-name, nor do they
> have a default_value.
> 
> So I think we should mark the display_name, display_name_language_code and
> default_value sysfs-attributes optional, e.g. make the description look like
> this:
> 
> 		default_value:	An optional file that can be read to obtain the
> 				default value of the <attr>
> 

At this point, why don't we just make a declaration at the top that all
attributes are optional?  If you want this to scale to non-BIOS implementations
of settings you couldn't know what they'll be able to offer and it sets
an expectation that userspace to need to look for every sysfs file exists rather
than just the vendor specific ones.

+
> > +		Drivers may emit a CHANGE uevent when a password is set or unset
> > +		userspace may check it again.
> 
> First of all some generic remarks:
> 
> Currently the "Admin" and "System" names come directly from the Dell WMI
> interface. I have 2 concerns with this:
> 
> 1) What if we do get multiple authentication mechanisms for a single user,
> e.g. both a type == "pasword" and type == "hotp" authentication. The way I
> have
> been thinking about that sofar, is that we then get 2 admin dirs under the
> /sys/class/firmware-attributes/*/authentication dir, with a type attribute
> per dir, following how we do the attributes. So we would get e.g. these 2
> dirs:
> 
> /sys/class/firmware-attributes/dell/authentication/admin-password
> /sys/class/firmware-attributes/dell/authentication/admin-hotp
> 
> For the admin user. If want to do it like this in the future we should
> add some indirection between the WMI username and the dir which is created
> now and create the Admin dir as admin-password starting now.

Yeah I think if HOTP is added to some vendor some day that's how it would work.
The indirection can be added at that time.  One way to do this is to add

> 
> 2) The "Admin" name is clear enough, but the "System" name is somewhat
> ambiguous and other vendors may call this differently, I think I have
> at least seen it called the "User" password in some cases and Lenovo
> seems to call it a power-on-password. I think that just calling it the
> "boot" password makes sense. My main concern is that "System" is a bit
> too vague. So then for now we would get:
> 
> /sys/class/firmware-attributes/dell/authentication/admin-password
> /sys/class/firmware-attributes/dell/authentication/boot-password

I want to be cognizant that vendors are going to call things differently
in their attributes and we want the specifications and/or whitepapers that
vendors use to refer to this to make sense to the system's administrator.

Dell uses the nomenclature "System" in all of it's documentation. If the Linux
implementation calls it "boot password" or "power on password" it will be
confusing to decode when someone see it. 

Dell also has other terms used such as Master password and HDD password.
They're not exposed in this interface but these all might have a different
connotation across vendors.

So instead I would propose that within the folder the "type" attribute
correspond to something decodable.  So the name the vendor uses is the
folder and the type of password is within a sysfs file "type".

Proposed types:
* "bios-admin"
* "power-on"

Those two types can then be hardcoded by the implementation.

So a user would see the different authentication mechanisms available
by looking At the contents of /sys/class/firmware-attributes/*/authentication
and if they don't understand it's purpose they look at the type sysfs file.

> 
> The spec. should also specify that the part before the first '-' is the
> username, and the part after it is the authentication type. E.g. the
> docs for this could look something like this:
> 
> 	Directories under /sys/class/firmware-attributes/*/authentication/
> 	use the following directory-name pattern:
> 	<username>-<authentication_method>
> 
> 	Where username must be one of: "admin" or "boot":

Username is inappropriate in this context, especially since firmware doesn't
have a concept of multi-user.  It's a configurable permissions scheme to what
you are allowed to do in firmware.

> 
> 	admin	If any authentication_method is enabled for the admin user, then
> 		authentication as the admin user is required to modify BIOS
> settings.
> 	boot	If any authentication_method is enabled for the admin user, then
> 		authentication as the boot user is required to boot the machine.
> 
> 	And where authentication_method must be "password". Note in the future
> 	both more usernames and more authentication_method-s may be added.
> 
> 	All authentication_methods must have the following sysfs-attributes:
> 
> 	is_enabled:  This reads "1" if the authentication_method is enabled,
> 		     and "0" if its disabled
> 
> 	Any changes to authentication_methods will generate a change uevent,
> 	upon receiving this event applications should recheck the authentication
> 	settings such as the is_enabled flag.
> 
> 	Password authentication_method specific sysfs-attributes:
> 
> 	max_password_length: ... (continue with the old text)
> 
> Note:
> 
> 1) This is a proposal to make the authentication bits a bit more generic /
>     future proof. This is very much open for discussion.
> 
> 2) The new generic is_enabled sysfs-attribute replaces the
> is_authentication_set flag
> 
> ###
> 
> So as with the actual firmware-attributes I have also been comparing the
> authentication
> bits for the Dell case with the community thinkpad_wmi code. And again things
> are a pretty
> good match already, including being able to query a minimum and maximin
> password length.
> 
> The only thing which is different, which I think would be good to add now, is
> a password_encoding sysfs-attribute. The Lenovo password interface supports
> 2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
> works on modern UEFI based platforms.
> 
> Since the Dell password code uses UTF8 to UTF16 translation routines, I guess
> the encoding for the Dell password is UTF8 (at the sysfs level). So I would
> like to propose
> an extra password-authentication attribute like:
> 
> 	password_encoding:  A file that can be read to obtain the encoding used
> by
> 			    the current_password and new_password attributes.
> 			    The value returned should be one of: "utf8", "ascii".
> 			    In some cases this may return a vendor-specific encoding
> 			    like, e.g. "lenovo-scancodes".
> 
> And for the Dell driver this would just be hardcoded to utf8.

I don't really believe that another vendor's implementation would be likely to
use scan codes for the input into the WMI method.

Think back to how this all works on Windows...

Here is the Dell whitepaper that supports this interface for Windows:
https://downloads.dell.com/manuals/common/dell-agentless-client-manageability.pdf

(And yes I acknowledge we should probably reference this and the PLDM spec it's based
From in the commit message)

You're supposed to get a WMI method which you can interact with from Powershell.
You get a reference to the namespace, and then you make a function call from something
in the binary MOF (which maps to an ACPI WM** method).

This is how a command using a password argument works on Windows (from that whitepaper)

$pwd = ”PASSWORD”
$encoder = New-Object System.Text.UTF8Encoding
$bytes = $encoder.GetBytes($pwd)
$BAI.SetAttribute(1,$bytes.Length,$bytes,"UefiNwStack","Disabled")

I don't see any other encoders other than Unicode in the System.Text namespace
https://docs.microsoft.com/en-us/dotnet/api/system.text?view=netcore-3.1

So if you had to manually convert to scancodes, that would not at all user friendly.

I would much prefer that this attribute only be added if it's actually deemed
necessary.  Or to my point that all attributes can be considered optional, Dell's
implementation would just not offer it.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-25 15:32   ` Limonciello, Mario
@ 2020-09-25 18:16     ` Hans de Goede
  2020-09-25 18:37       ` Limonciello, Mario
  0 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2020-09-25 18:16 UTC (permalink / raw)
  To: Limonciello, Mario, Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Bharathi, Divya, Andy Shevchenko,
	mark gross, Ksr, Prasanth, Mark Pearson

Hi,

On 9/25/20 5:32 PM, Limonciello, Mario wrote:
> So I do want to preface this response by mentioning that Dell's implementation
> is based off the PLDM specification from the DMTF.
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0247_1.0.0.pdf
> 
> A lot of the nomenclature that has been already proposed to change followed
> nomenclature previously used in the PLDM specification.  In the spirit of matching
> that specification in the Linux implementation we may consider to change things
> like proposed to be renamed min_value back to lower_bound, but that's up to your
> decision.

I really prefer min_value and max_value here, those have 2 advantages:

1. min/max is used almost everywhere in the kernel/sysfs for things like this,
    upper-/lower-bound feels a bit like archaic Enlish to me.

2. The _value postfix makes it immediately clear that they are the bounds for
    the current_value attribute.

Also in case of the password sysfs inteface min/max_lenght is used, not
e.g. password_length_lower_bound. So again using min/max seems more consistent.

>> This list seems to miss a "type" sysfs attribute, which tells which type
>> the firmware-attribute in question is. Sorry for not noticing that sooner.
> 
> Whoops :)
> 
>>
>>> +
>>> +		current_value:	A file that can be read to obtain the current
>>> +		value of the <attr>
>>
>> Can you indent the continuation on the second line to align with the start
>> of the description on the first line please?, e.g.:
>>
>> 		current_value:	A file that can be read to obtain the current
>> 				value of the <attr>
>>
>> This goes for all the sysfs-attribute description texts.
>>
>>> +
>>> +		This file can also be written to in order to update
>>> +		the value of a <attr>
>>> +
>>> +		default_value:	A file that can be read to obtain the default
>>> +		value of the <attr>
>>> +
>>> +		display_name:	A file that can be read to obtain a user friendly
>>> +		description of the at <attr>
>>> +
>>> +		display_name_language_code:	A file that can be read to obtain
>>> +		the IETF language tag corresponding to the "display_name" of the
>> <attr>
>>> +
>>> +		"enumeration"-type specific properties:
>>> +
>>> +		possible_values: A file that can be read to obtain the possible
>>> +		values of the <attr>. Values are separated using semi-colon.
>>
>> As non-native English speaker I had to lookup semi-colon to make sure that
>> it indeed is ';' as I already sorta expected. So can we add "(';')" (without
>> the "") behind the semi-colon to make this easier to parse for non-native
>> English
>> speakers?
> 
> 
> Some parts of the kernel documentation seem to use semi-colon (``;``) - example of
> admin-guide/bootconfig.rst
> 
> and others just set semi-colon - example of admin-guide/device-mapper/dm-init.rst.
> 
> But I don't see a problem with this change.
> 
>>
>>> +		"integer"-type specific properties:
>>> +
>>> +		min_value:	A file that can be read to obtain the lower
>>> +		bound value of the <attr>
>>> +
>>> +		max_value:	A file that can be read to obtain the upper
>>> +		bound value of the <attr>
>>> +
>>> +		scalar_increment:	A file that can be read to obtain the
>>> +		resolution of the incremental value this attribute accepts.
>>
>> Can we have an example here? I guess if for some reason only even/odd values
>> are allowed then this would contain "2" ?
> 
> Maybe adopting the PLDM description is better to explain it.
> 
> "The scalar value that is used for the increments to this integer"
>>
>>> +
>>> +		"string"-type specific properties:
>>> +
>>> +		max_length:	A file that can be read to obtain the maximum
>>> +		length value of the <attr>
>>> +
>>> +		min_length:	A file that can be read to obtain the minimum
>>> +		length value of the <attr>
>>
>> So I have been taking a look at a community written driven to allow changing
>> BIOS-settings / firmware-attributes on some Lenovo laptops:
>>
>> https://github.com/iksaif/thinkpad-wmi
>>
>> My main reason for doing so is to check if other vendors also support things
>> like "display_name", "default_value", etc.
>>
>> So for the normal attributes, it seems that for the Thinkpad WMI interface
>> they
>> are all enums and the do contain possible_values. So that is fine.
>>
>> But they do not have a separate display_name only a name-name, nor do they
>> have a default_value.
>>
>> So I think we should mark the display_name, display_name_language_code and
>> default_value sysfs-attributes optional, e.g. make the description look like
>> this:
>>
>> 		default_value:	An optional file that can be read to obtain the
>> 				default value of the <attr>
>>
> 
> At this point, why don't we just make a declaration at the top that all
> attributes are optional?  If you want this to scale to non-BIOS implementations
> of settings you couldn't know what they'll be able to offer and it sets
> an expectation that userspace to need to look for every sysfs file exists rather
> than just the vendor specific ones.

Sure I would be happy to have it documented that all firmware-attributes sysfs-files
are optional *except for type and current_value*. If you prefer that go for it.

> 
> +
>>> +		Drivers may emit a CHANGE uevent when a password is set or unset
>>> +		userspace may check it again.
>>
>> First of all some generic remarks:
>>
>> Currently the "Admin" and "System" names come directly from the Dell WMI
>> interface. I have 2 concerns with this:
>>
>> 1) What if we do get multiple authentication mechanisms for a single user,
>> e.g. both a type == "pasword" and type == "hotp" authentication. The way I
>> have
>> been thinking about that sofar, is that we then get 2 admin dirs under the
>> /sys/class/firmware-attributes/*/authentication dir, with a type attribute
>> per dir, following how we do the attributes. So we would get e.g. these 2
>> dirs:
>>
>> /sys/class/firmware-attributes/dell/authentication/admin-password
>> /sys/class/firmware-attributes/dell/authentication/admin-hotp
>>
>> For the admin user. If want to do it like this in the future we should
>> add some indirection between the WMI username and the dir which is created
>> now and create the Admin dir as admin-password starting now.
> 
> Yeah I think if HOTP is added to some vendor some day that's how it would work.
> The indirection can be added at that time.  One way to do this is to add

It seems you never finished your sentence here?

>> 2) The "Admin" name is clear enough, but the "System" name is somewhat
>> ambiguous and other vendors may call this differently, I think I have
>> at least seen it called the "User" password in some cases and Lenovo
>> seems to call it a power-on-password. I think that just calling it the
>> "boot" password makes sense. My main concern is that "System" is a bit
>> too vague. So then for now we would get:
>>
>> /sys/class/firmware-attributes/dell/authentication/admin-password
>> /sys/class/firmware-attributes/dell/authentication/boot-password
> 
> I want to be cognizant that vendors are going to call things differently
> in their attributes and we want the specifications and/or whitepapers that
> vendors use to refer to this to make sense to the system's administrator.
> 
> Dell uses the nomenclature "System" in all of it's documentation. If the Linux
> implementation calls it "boot password" or "power on password" it will be
> confusing to decode when someone see it.
> 
> Dell also has other terms used such as Master password and HDD password.
> They're not exposed in this interface but these all might have a different
> connotation across vendors.
> 
> So instead I would propose that within the folder the "type" attribute
> correspond to something decodable.  So the name the vendor uses is the
> folder and the type of password is within a sysfs file "type".
> 
> Proposed types:
> * "bios-admin"
> * "power-on"
> 
> Those two types can then be hardcoded by the implementation.

If re-using the WMI names is important for you, then having a sysfs-attribute
with some standardized value to say what is what inside the authentication-sub-dir
is fine with me.

Except that I would not call it type, when thinking about authentication-types
I think about things like password / totp / hotp. Can we call the sysfs-attribute
for this "role" instead ?

> So a user would see the different authentication mechanisms available
> by looking At the contents of /sys/class/firmware-attributes/*/authentication
> and if they don't understand it's purpose they look at the type sysfs file.

But one role can still have multiple mechanisms, so for Dell in the future
we could have say:

/sys/class/firmware-attributes/dell/authentication/Admin-password
/sys/class/firmware-attributes/dell/authentication/Admin-hotp
/sys/class/firmware-attributes/dell/authentication/System-password

So although I'm fine with taking the role_name directly from WMI
(combined with a roll attribute with standardized values) I think
we still need to postfix a -password to it now, to allow room
for adding say a -hotp mechanism for the same role_name in the
future ?

(I guess this also ties into your unanswered question from above.

>> The spec. should also specify that the part before the first '-' is the
>> username, and the part after it is the authentication type. E.g. the
>> docs for this could look something like this:
>>
>> 	Directories under /sys/class/firmware-attributes/*/authentication/
>> 	use the following directory-name pattern:
>> 	<username>-<authentication_method>
>>
>> 	Where username must be one of: "admin" or "boot":
> 
> Username is inappropriate in this context, especially since firmware doesn't
> have a concept of multi-user.  It's a configurable permissions scheme to what
> you are allowed to do in firmware.

Ack, so as I asked above, what do you think of using "role" here instead?


>> 	admin	If any authentication_method is enabled for the admin user, then
>> 		authentication as the admin user is required to modify BIOS
>> settings.
>> 	boot	If any authentication_method is enabled for the admin user, then
>> 		authentication as the boot user is required to boot the machine.
>>
>> 	And where authentication_method must be "password". Note in the future
>> 	both more usernames and more authentication_method-s may be added.
>>
>> 	All authentication_methods must have the following sysfs-attributes:
>>
>> 	is_enabled:  This reads "1" if the authentication_method is enabled,
>> 		     and "0" if its disabled
>>
>> 	Any changes to authentication_methods will generate a change uevent,
>> 	upon receiving this event applications should recheck the authentication
>> 	settings such as the is_enabled flag.
>>
>> 	Password authentication_method specific sysfs-attributes:
>>
>> 	max_password_length: ... (continue with the old text)
>>
>> Note:
>>
>> 1) This is a proposal to make the authentication bits a bit more generic /
>>      future proof. This is very much open for discussion.
>>
>> 2) The new generic is_enabled sysfs-attribute replaces the
>> is_authentication_set flag
>>
>> ###
>>
>> So as with the actual firmware-attributes I have also been comparing the
>> authentication
>> bits for the Dell case with the community thinkpad_wmi code. And again things
>> are a pretty
>> good match already, including being able to query a minimum and maximin
>> password length.
>>
>> The only thing which is different, which I think would be good to add now, is
>> a password_encoding sysfs-attribute. The Lenovo password interface supports
>> 2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
>> works on modern UEFI based platforms.
>>
>> Since the Dell password code uses UTF8 to UTF16 translation routines, I guess
>> the encoding for the Dell password is UTF8 (at the sysfs level). So I would
>> like to propose
>> an extra password-authentication attribute like:
>>
>> 	password_encoding:  A file that can be read to obtain the encoding used
>> by
>> 			    the current_password and new_password attributes.
>> 			    The value returned should be one of: "utf8", "ascii".
>> 			    In some cases this may return a vendor-specific encoding
>> 			    like, e.g. "lenovo-scancodes".
>>
>> And for the Dell driver this would just be hardcoded to utf8.
> 
> I don't really believe that another vendor's implementation would be likely to
> use scan codes for the input into the WMI method.

I did not make that example up, Lenovo really as a scan-codes encoding for
their password authentication mechanism, see:

https://download.lenovo.com/pccbbs/mobiles_pdf/kbl-r_deploy_01.pdf

> Think back to how this all works on Windows...

<snip>

> So if you had to manually convert to scancodes, that would not at all user friendly.

It definitely is not user-friendly.

Note when calling the Lenovo WMI functions you can specify in which encoding
(ascii or scancodes) you are providing the data and all Lenovo's examples
use the ascii encoding.  But there also is a bitfield with supported encodings
and I guess some older firmware may only support the scancodes variant?

It is all somewhat not nice, which is why I prefixed the scancodes encoding
with lenovo- to make it clear that it is non-standard.

But lets say all Lenovo models support the ascii encoding and we never expose
the scancodes bit to userspace. Even then there still is the unicode (utf8
in sysfs, utf16 at the WMI level for Dell) vs ascii issue and it would be nice
if a UI for this could give an error when the user tries to use non ascii
chars in a password for a vendor implementation...

> I would much prefer that this attribute only be added if it's actually deemed
> necessary.  Or to my point that all attributes can be considered optional, Dell's
> implementation would just not offer it.

I guess that would work (Dell not offering it). Which makes me realize that
we should specify somewhere in the doc that all sysfs files which contain
a string value, the encoding is UTF8 unless explicitly specified otherwise.
(for that specific sysfs-attribute).

Regards,

Hans


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-25 18:16     ` Hans de Goede
@ 2020-09-25 18:37       ` Limonciello, Mario
  2020-09-25 19:40         ` Hans de Goede
  0 siblings, 1 reply; 9+ messages in thread
From: Limonciello, Mario @ 2020-09-25 18:37 UTC (permalink / raw)
  To: Hans de Goede, Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Bharathi, Divya, Andy Shevchenko,
	mark gross, Ksr, Prasanth, Mark Pearson

> 
> I really prefer min_value and max_value here, those have 2 advantages:
> 
> 1. min/max is used almost everywhere in the kernel/sysfs for things like this,
>     upper-/lower-bound feels a bit like archaic Enlish to me.
> 
> 2. The _value postfix makes it immediately clear that they are the bounds for
>     the current_value attribute.
> 
> Also in case of the password sysfs inteface min/max_lenght is used, not
> e.g. password_length_lower_bound. So again using min/max seems more
> consistent.

OK.

> 
> Sure I would be happy to have it documented that all firmware-attributes
> sysfs-files
> are optional *except for type and current_value*. If you prefer that go for
> it.

OK.

> 
> >
> > +
> >>> +		Drivers may emit a CHANGE uevent when a password is set or unset
> >>> +		userspace may check it again.
> >>
> >> First of all some generic remarks:
> >>
> >> Currently the "Admin" and "System" names come directly from the Dell WMI
> >> interface. I have 2 concerns with this:
> >>
> >> 1) What if we do get multiple authentication mechanisms for a single user,
> >> e.g. both a type == "pasword" and type == "hotp" authentication. The way I
> >> have
> >> been thinking about that sofar, is that we then get 2 admin dirs under the
> >> /sys/class/firmware-attributes/*/authentication dir, with a type attribute
> >> per dir, following how we do the attributes. So we would get e.g. these 2
> >> dirs:
> >>
> >> /sys/class/firmware-attributes/dell/authentication/admin-password
> >> /sys/class/firmware-attributes/dell/authentication/admin-hotp
> >>
> >> For the admin user. If want to do it like this in the future we should
> >> add some indirection between the WMI username and the dir which is created
> >> now and create the Admin dir as admin-password starting now.
> >
> > Yeah I think if HOTP is added to some vendor some day that's how it would
> work.
> > The indirection can be added at that time.  One way to do this is to add
> 
> It seems you never finished your sentence here?

Whoops - yeah I was jumping around in my response and forgot to finish my thought.

I was going to suggest that if necessary a compatible way to add these would
be symlinks.  So if the directory right now was Admin and later was had
split to something like AdminHotp/AdminPassword
but wanted to discriminate between the old Admin it could be Admin->AdminPassword
or vice versa.

> 
> >> 2) The "Admin" name is clear enough, but the "System" name is somewhat
> >> ambiguous and other vendors may call this differently, I think I have
> >> at least seen it called the "User" password in some cases and Lenovo
> >> seems to call it a power-on-password. I think that just calling it the
> >> "boot" password makes sense. My main concern is that "System" is a bit
> >> too vague. So then for now we would get:
> >>
> >> /sys/class/firmware-attributes/dell/authentication/admin-password
> >> /sys/class/firmware-attributes/dell/authentication/boot-password
> >
> > I want to be cognizant that vendors are going to call things differently
> > in their attributes and we want the specifications and/or whitepapers that
> > vendors use to refer to this to make sense to the system's administrator.
> >
> > Dell uses the nomenclature "System" in all of it's documentation. If the
> Linux
> > implementation calls it "boot password" or "power on password" it will be
> > confusing to decode when someone see it.
> >
> > Dell also has other terms used such as Master password and HDD password.
> > They're not exposed in this interface but these all might have a different
> > connotation across vendors.
> >
> > So instead I would propose that within the folder the "type" attribute
> > correspond to something decodable.  So the name the vendor uses is the
> > folder and the type of password is within a sysfs file "type".
> >
> > Proposed types:
> > * "bios-admin"
> > * "power-on"
> >
> > Those two types can then be hardcoded by the implementation.
> 
> If re-using the WMI names is important for you, then having a sysfs-attribute
> with some standardized value to say what is what inside the authentication-
> sub-dir
> is fine with me.
> 
> Except that I would not call it type, when thinking about authentication-types
> I think about things like password / totp / hotp. Can we call the sysfs-
> attribute
> for this "role" instead ?

That sounds good to me.

> 
> > So a user would see the different authentication mechanisms available
> > by looking At the contents of /sys/class/firmware-
> attributes/*/authentication
> > and if they don't understand it's purpose they look at the type sysfs file.
> 
> But one role can still have multiple mechanisms, so for Dell in the future
> we could have say:
> 
> /sys/class/firmware-attributes/dell/authentication/Admin-password
> /sys/class/firmware-attributes/dell/authentication/Admin-hotp
> /sys/class/firmware-attributes/dell/authentication/System-password
> 
> So although I'm fine with taking the role_name directly from WMI
> (combined with a roll attribute with standardized values) I think
> we still need to postfix a -password to it now, to allow room
> for adding say a -hotp mechanism for the same role_name in the
> future ?

Could this be captured in the role attribute instead perhaps?  So the role
attributes values could hypothetically be:
bios-admin-password
power-on-password

And if HOTP is added some day these could be added:
bios-admin-hotp
power-on-hotp

> 
> (I guess this also ties into your unanswered question from above.
> 
> >> The spec. should also specify that the part before the first '-' is the
> >> username, and the part after it is the authentication type. E.g. the
> >> docs for this could look something like this:
> >>
> >> 	Directories under /sys/class/firmware-attributes/*/authentication/
> >> 	use the following directory-name pattern:
> >> 	<username>-<authentication_method>
> >>
> >> 	Where username must be one of: "admin" or "boot":
> >
> > Username is inappropriate in this context, especially since firmware doesn't
> > have a concept of multi-user.  It's a configurable permissions scheme to
> what
> > you are allowed to do in firmware.
> 
> Ack, so as I asked above, what do you think of using "role" here instead?
> 

+1

> 
> >> 	admin	If any authentication_method is enabled for the admin user, then
> >> 		authentication as the admin user is required to modify BIOS
> >> settings.
> >> 	boot	If any authentication_method is enabled for the admin user, then
> >> 		authentication as the boot user is required to boot the machine.
> >>
> >> 	And where authentication_method must be "password". Note in the future
> >> 	both more usernames and more authentication_method-s may be added.
> >>
> >> 	All authentication_methods must have the following sysfs-attributes:
> >>
> >> 	is_enabled:  This reads "1" if the authentication_method is enabled,
> >> 		     and "0" if its disabled
> >>
> >> 	Any changes to authentication_methods will generate a change uevent,
> >> 	upon receiving this event applications should recheck the authentication
> >> 	settings such as the is_enabled flag.
> >>
> >> 	Password authentication_method specific sysfs-attributes:
> >>
> >> 	max_password_length: ... (continue with the old text)
> >>
> >> Note:
> >>
> >> 1) This is a proposal to make the authentication bits a bit more generic /
> >>      future proof. This is very much open for discussion.
> >>
> >> 2) The new generic is_enabled sysfs-attribute replaces the
> >> is_authentication_set flag
> >>
> >> ###
> >>
> >> So as with the actual firmware-attributes I have also been comparing the
> >> authentication
> >> bits for the Dell case with the community thinkpad_wmi code. And again
> things
> >> are a pretty
> >> good match already, including being able to query a minimum and maximin
> >> password length.
> >>
> >> The only thing which is different, which I think would be good to add now,
> is
> >> a password_encoding sysfs-attribute. The Lenovo password interface supports
> >> 2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
> >> works on modern UEFI based platforms.
> >>
> >> Since the Dell password code uses UTF8 to UTF16 translation routines, I
> guess
> >> the encoding for the Dell password is UTF8 (at the sysfs level). So I would
> >> like to propose
> >> an extra password-authentication attribute like:
> >>
> >> 	password_encoding:  A file that can be read to obtain the encoding used
> >> by
> >> 			    the current_password and new_password attributes.
> >> 			    The value returned should be one of: "utf8", "ascii".
> >> 			    In some cases this may return a vendor-specific encoding
> >> 			    like, e.g. "lenovo-scancodes".
> >>
> >> And for the Dell driver this would just be hardcoded to utf8.
> >
> > I don't really believe that another vendor's implementation would be likely
> to
> > use scan codes for the input into the WMI method.
> 
> I did not make that example up, Lenovo really as a scan-codes encoding for
> their password authentication mechanism, see:
> 
> https://download.lenovo.com/pccbbs/mobiles_pdf/kbl-r_deploy_01.pdf
> 
> > Think back to how this all works on Windows...
> 
> <snip>
> 
> > So if you had to manually convert to scancodes, that would not at all user
> friendly.
> 
> It definitely is not user-friendly.
> 
> Note when calling the Lenovo WMI functions you can specify in which encoding
> (ascii or scancodes) you are providing the data and all Lenovo's examples
> use the ascii encoding.  But there also is a bitfield with supported encodings
> and I guess some older firmware may only support the scancodes variant?
> 
> It is all somewhat not nice, which is why I prefixed the scancodes encoding
> with lenovo- to make it clear that it is non-standard.
> 
> But lets say all Lenovo models support the ascii encoding and we never expose
> the scancodes bit to userspace. 

The documentation you linked doesn't seem to indicate when to use scancodes or
ASCII to me, so I can't draw any conclusions if certain models support one or
the other.

I would suggest yes please don't support scancodes from the sysfs perspective for
another vendor's implementation.  We should probably keep sysfs as utf8 and let
any conversions be hidden in the kernel driver if necessary. 

> Even then there still is the unicode (utf8
> in sysfs, utf16 at the WMI level for Dell) vs ascii issue and it would be nice
> if a UI for this could give an error when the user tries to use non ascii
> chars in a password for a vendor implementation...

I think that the kernel driver can certainly parse and provide -EINVAL in this
context. 

> 
> > I would much prefer that this attribute only be added if it's actually
> deemed
> > necessary.  Or to my point that all attributes can be considered optional,
> Dell's
> > implementation would just not offer it.
> 
> I guess that would work (Dell not offering it). Which makes me realize that
> we should specify somewhere in the doc that all sysfs files which contain
> a string value, the encoding is UTF8 unless explicitly specified otherwise.
> (for that specific sysfs-attribute).

Yeah.  I guess the very top where we will modify to mention that all attributes
are optional we can also mention that the encoding is UTF8 unless otherwise
specified.

Andy - 

BTW - with the maintainer change in platform-x86 should Divya stop CC you and
Darren?





^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems
  2020-09-25 18:37       ` Limonciello, Mario
@ 2020-09-25 19:40         ` Hans de Goede
  0 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2020-09-25 19:40 UTC (permalink / raw)
  To: Limonciello, Mario, Divya Bharathi, dvhart
  Cc: LKML, platform-driver-x86, Bharathi, Divya, Andy Shevchenko,
	mark gross, Ksr, Prasanth, Mark Pearson

Hi,

On 9/25/20 8:37 PM, Limonciello, Mario wrote:

<snip>

>>> So a user would see the different authentication mechanisms available
>>> by looking At the contents of /sys/class/firmware-
>> attributes/*/authentication
>>> and if they don't understand it's purpose they look at the type sysfs file.
>>
>> But one role can still have multiple mechanisms, so for Dell in the future
>> we could have say:
>>
>> /sys/class/firmware-attributes/dell/authentication/Admin-password
>> /sys/class/firmware-attributes/dell/authentication/Admin-hotp
>> /sys/class/firmware-attributes/dell/authentication/System-password
>>
>> So although I'm fine with taking the role_name directly from WMI
>> (combined with a roll attribute with standardized values) I think
>> we still need to postfix a -password to it now, to allow room
>> for adding say a -hotp mechanism for the same role_name in the
>> future ?
> 
> Could this be captured in the role attribute instead perhaps?  So the role
> attributes values could hypothetically be:
> bios-admin-password
> power-on-password
> 
> And if HOTP is added some day these could be added:
> bios-admin-hotp
> power-on-hotp

I would rather have the auth-mechanism-type in a separate mechanism sysfs-file
which could then contain e.g. "password" or "hotp".

But that does not solve the bit which I'm worried about,
what I'm worried about is a future scenario where there are multiple
auth mechanisms for the Admin role, and assuming we use one dir per
auth-mechanism then we would need 2 Admin sub-dirs which is not
allowed of course.

I assume that that is what the symlink suggestion you did:

 > I was going to suggest that if necessary a compatible way to add these would
 > be symlinks.  So if the directory right now was Admin and later was had
 > split to something like AdminHotp/AdminPassword
 > but wanted to discriminate between the old Admin it could be Admin->AdminPassword
 > or vice versa.

Is meant to address. So for now we go with just Admin and then if
the 2 auth-mechanisms for Admin at the same time scenario happens
add the "-<mechanism>" suffix to the directory name at that point.
Or I guess we could even add a ".%d" suffix for duplicates, since
the mechanism info would already be in the separate mechanism
sysfs file.  Then we can also avoid the symlink since we can just
leave out the .%d suffix when the integer for %d is 0, there is
precedence for that.

So TL;DR: yes we could put the mechanism in a sysfs file too,
in that case I would prefer to go with a separate file though,
rather then concatenating it to the role and storing it in the
role sysfs file.

If we have both the role and mechanism (hardcoded to "password"
for now) as sysfs-files in say a:

/sys/class/firmware-attributes/dell/authentication/Admin

Dir then indeed we will not need the "-password" suffix. And if
we get more then one auth-mechanism for say the "Admin" role then
we can just name the second subdir Admin.1, etc.

So compared to the current sysfs API from the v4 patch that would
mean adding the role ("admin", or "power-on") and mechanism
(always "password" for now) sysfs files and otherwise no changes.

Oh and can we rename the "is_authentication_set" sysfs file to "is_enabled"
please?  Being set is well defined for a password, but not so much for possible
other authentication-mechanisms.

<snip>

>>>> ###
>>>>
>>>> So as with the actual firmware-attributes I have also been comparing the
>>>> authentication
>>>> bits for the Dell case with the community thinkpad_wmi code. And again
>> things
>>>> are a pretty
>>>> good match already, including being able to query a minimum and maximin
>>>> password length.
>>>>
>>>> The only thing which is different, which I think would be good to add now,
>> is
>>>> a password_encoding sysfs-attribute. The Lenovo password interface supports
>>>> 2 encodings, "ascii" and "scancodes". Although I wonder if scancodes still
>>>> works on modern UEFI based platforms.
>>>>
>>>> Since the Dell password code uses UTF8 to UTF16 translation routines, I
>> guess
>>>> the encoding for the Dell password is UTF8 (at the sysfs level). So I would
>>>> like to propose
>>>> an extra password-authentication attribute like:
>>>>
>>>> 	password_encoding:  A file that can be read to obtain the encoding used
>>>> by
>>>> 			    the current_password and new_password attributes.
>>>> 			    The value returned should be one of: "utf8", "ascii".
>>>> 			    In some cases this may return a vendor-specific encoding
>>>> 			    like, e.g. "lenovo-scancodes".
>>>>
>>>> And for the Dell driver this would just be hardcoded to utf8.
>>>
>>> I don't really believe that another vendor's implementation would be likely
>> to
>>> use scan codes for the input into the WMI method.
>>
>> I did not make that example up, Lenovo really as a scan-codes encoding for
>> their password authentication mechanism, see:
>>
>> https://download.lenovo.com/pccbbs/mobiles_pdf/kbl-r_deploy_01.pdf
>>

<snip>

> The documentation you linked doesn't seem to indicate when to use scancodes or
> ASCII to me, so I can't draw any conclusions if certain models support one or
> the other.
> 
> I would suggest yes please don't support scancodes from the sysfs perspective for
> another vendor's implementation.  We should probably keep sysfs as utf8 and let
> any conversions be hidden in the kernel driver if necessary.

Doing the conversions in kernel is not really ideal, the kernels i18n support
is very limited. But yes for utf8->ascii and utf8->utf16 we can do the conversion
in kernel.

>> Even then there still is the unicode (utf8
>> in sysfs, utf16 at the WMI level for Dell) vs ascii issue and it would be nice
>> if a UI for this could give an error when the user tries to use non ascii
>> chars in a password for a vendor implementation...
> 
> I think that the kernel driver can certainly parse and provide -EINVAL in this
> context.

True, but the advantage of having userspace know it needs to be ascii is
that it can provide a much more sensible error message to the user when
the user tries to use non ascii in the password.

So I think that in the non utf8 case it would still be good to have
a password_encoding file. With that said, as discussed this sysfs-file
will be optional too, so for the "Systems Management Driver over WMI for Dell
Systems" we can just leave it out and then revisit when we merge a driver
which does not support utf8 for passwords.

>>> I would much prefer that this attribute only be added if it's actually
>> deemed
>>> necessary.  Or to my point that all attributes can be considered optional,
>> Dell's
>>> implementation would just not offer it.
>>
>> I guess that would work (Dell not offering it). Which makes me realize that
>> we should specify somewhere in the doc that all sysfs files which contain
>> a string value, the encoding is UTF8 unless explicitly specified otherwise.
>> (for that specific sysfs-attribute).
> 
> Yeah.  I guess the very top where we will modify to mention that all attributes
> are optional we can also mention that the encoding is UTF8 unless otherwise
> specified.

Ack.

Regards,

Hans


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2020-09-25 20:13 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-23 11:30 [PATCH v4] Introduce support for Systems Management Driver over WMI for Dell Systems Divya Bharathi
2020-09-23 17:11 ` Randy Dunlap
2020-09-23 18:39   ` Limonciello, Mario
2020-09-23 18:41     ` Randy Dunlap
2020-09-25 13:26 ` Hans de Goede
2020-09-25 15:32   ` Limonciello, Mario
2020-09-25 18:16     ` Hans de Goede
2020-09-25 18:37       ` Limonciello, Mario
2020-09-25 19:40         ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).