LinuxLists.cc - [PATCH v11 00/14] HP BIOSCFG driver

2023-04-20 16:55:59

by Jorge Lopez

[permalink] [raw]

Subject: [PATCH v11 00/14] HP BIOSCFG driver

2023-04-20 16:56:01

by Jorge Lopez

[permalink] [raw]

2023-04-20 16:56:13

by Jorge Lopez

[permalink] [raw]

Subject: [PATCH v11 06/14] HP BIOSCFG driver - passwdobj-attributes

HP BIOS Configuration driver purpose is to provide a driver supporting
the latest sysfs class firmware attributes framework allowing the user
to change BIOS settings and security solutions on HP Inc.’s commercial
notebooks.

Many features of HP Commercial notebooks can be managed using Windows
Management Instrumentation (WMI). WMI is an implementation of Web-Based
Enterprise Management (WBEM) that provides a standards-based interface
for changing and monitoring system settings. HP BIOSCFG driver provides
a native Linux solution and the exposed features facilitates the
migration to Linux environments.

The Linux security features to be provided in hp-bioscfg driver enables
managing the BIOS settings and security solutions via sysfs, a virtual
filesystem that can be used by user-mode applications. The new
documentation cover HP-specific firmware sysfs attributes such Secure
Platform Management and Sure Start. Each section provides security
feature description and identifies sysfs directories and files exposed
by the driver.

Many HP Commercial notebooks include a feature called Secure Platform
Management (SPM), which replaces older password-based BIOS settings
management with public key cryptography. PC secure product management
begins when a target system is provisioned with cryptographic keys
that are used to ensure the integrity of communications between system
management utilities and the BIOS.

HP Commercial notebooks have several BIOS settings that control its
behaviour and capabilities, many of which are related to security.
To prevent unauthorized changes to these settings, the system can
be configured to use a cryptographic signature-based authorization
string that the BIOS will use to verify authorization to modify the
setting.

Linux Security components are under development and not published yet.
The only linux component is the driver (hp bioscfg) at this time.
Other published security components are under Windows.

Signed-off-by: Jorge Lopez <[email protected]>

---
Based on the latest platform-drivers-x86.git/for-next
---
.../x86/hp/hp-bioscfg/passwdobj-attributes.c | 669 ++++++++++++++++++
1 file changed, 669 insertions(+)
create mode 100644 drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c

diff --git a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
new file mode 100644
index 000000000000..c03b3a71e9c4
--- /dev/null
+++ b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
@@ -0,0 +1,669 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to password object type attributes under
+ * BIOS PASSWORD for use with hp-bioscfg driver.
+ *
+ * Copyright (c) 2022 HP Development Company, L.P.
+ */
+
+#include "bioscfg.h"
+#include <asm-generic/posix_types.h>
+
+GET_INSTANCE_ID(password);
+/*
+ * Clear all passwords copied to memory for a particular
+ * authentication instance
+ */
+int clear_passwords(const int instance)
+{
+ if (!bioscfg_drv.password_data[instance].is_enabled)
+ return 0;
+
+ memset(bioscfg_drv.password_data[instance].current_password,
+ 0, sizeof(bioscfg_drv.password_data[instance].current_password));
+ memset(bioscfg_drv.password_data[instance].new_password,
+ 0, sizeof(bioscfg_drv.password_data[instance].new_password));
+
+ return 0;
+}
+
+/*
+ * Clear all credentials copied to memory for both Power-ON and Setup
+ * BIOS instances
+ */
+int clear_all_credentials(void)
+{
+ int instance;
+
+ /* clear all passwords */
+ for (instance = 0; instance < bioscfg_drv.password_instances_count; instance++)
+ clear_passwords(instance);
+
+ /* clear auth_token */
+ kfree(bioscfg_drv.spm_data.auth_token);
+ bioscfg_drv.spm_data.auth_token = NULL;
+
+ return 0;
+}
+
+int get_password_instance_for_type(const char *name)
+{
+ int count = bioscfg_drv.password_instances_count;
+ int instance;
+
+ for (instance = 0; instance < count; instance++) {
+ if (strcmp(bioscfg_drv.password_data[instance].common.display_name, name) == 0)
+ return instance;
+ }
+ return -EINVAL;
+}
+
+int validate_password_input(int instance_id, const char *buf)
+{
+ int length;
+
+ length = strlen(buf);
+ if (buf[length-1] == '\n')
+ length--;
+
+ if (length > MAX_PASSWD_SIZE)
+ return INVALID_BIOS_AUTH;
+
+ if (bioscfg_drv.password_data[instance_id].min_password_length > length ||
+ bioscfg_drv.password_data[instance_id].max_password_length < length)
+ return INVALID_BIOS_AUTH;
+ return SUCCESS;
+}
+
+int password_is_set(const char *name)
+{
+ int id;
+
+ id = get_password_instance_for_type(name);
+ if (id < 0)
+ return 0;
+
+ return bioscfg_drv.password_data[id].is_enabled;
+}
+
+ATTRIBUTE_N_PROPERTY_SHOW(is_enabled, password);
+static struct kobj_attribute password_is_password_set = __ATTR_RO(is_enabled);
+
+static ssize_t current_password_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ char *p, *buf_cp;
+ int id, ret = 0;
+
+ buf_cp = kstrdup(buf, GFP_KERNEL);
+ if (!buf_cp) {
+ ret = -ENOMEM;
+ goto exit_password;
+ }
+
+ p = memchr(buf_cp, '\n', count);
+
+ if (p != NULL)
+ *p = '\0';
+
+ id = get_password_instance_id(kobj);
+
+ if (id >= 0)
+ ret = validate_password_input(id, buf_cp);
+
+ if (!ret) {
+ strscpy(bioscfg_drv.password_data[id].current_password,
+ buf_cp,
+ sizeof(bioscfg_drv.password_data[id].current_password));
+ /*
+ * set pending reboot flag depending on
+ * "RequiresPhysicalPresence" value
+ */
+ if (bioscfg_drv.password_data[id].common.requires_physical_presence)
+ bioscfg_drv.pending_reboot = true;
+ }
+
+exit_password:
+ kfree(buf_cp);
+ return ret ? ret : count;
+}
+static struct kobj_attribute password_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, *buf_cp = NULL;
+ int id = 0;
+ int ret = -EIO;
+
+ buf_cp = kstrdup(buf, GFP_KERNEL);
+ if (!buf_cp) {
+ ret = -ENOMEM;
+ goto exit_password;
+ }
+
+ p = memchr(buf_cp, '\n', count);
+
+ if (p != NULL)
+ *p = '\0';
+
+ id = get_password_instance_id(kobj);
+
+ if (id >= 0)
+ ret = validate_password_input(id, buf_cp);
+
+ if (!ret)
+ strscpy(bioscfg_drv.password_data[id].new_password,
+ buf_cp,
+ sizeof(bioscfg_drv.password_data[id].new_password));
+
+ if (!ret)
+ ret = hp_set_attribute(kobj->name, buf_cp);
+
+exit_password:
+ /*
+ * Regardless of the results both new and current passwords
+ * will be set to zero and avoid security issues
+ */
+ clear_passwords(id);
+
+ kfree(buf_cp);
+ return ret ? ret : count;
+}
+
+static struct kobj_attribute password_new_password = __ATTR_WO(new_password);
+
+
+ATTRIBUTE_N_PROPERTY_SHOW(min_password_length, password);
+static struct kobj_attribute password_min_password_length = __ATTR_RO(min_password_length);
+
+ATTRIBUTE_N_PROPERTY_SHOW(max_password_length, password);
+static struct kobj_attribute password_max_password_length = __ATTR_RO(max_password_length);
+
+static ssize_t role_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ if (strcmp(kobj->name, SETUP_PASSWD) == 0)
+ return sysfs_emit(buf, "%s\n", BIOS_ADMIN);
+
+ if (strcmp(kobj->name, POWER_ON_PASSWD) == 0)
+ return sysfs_emit(buf, "%s\n", POWER_ON);
+
+ return -EIO;
+}
+static struct kobj_attribute password_role = __ATTR_RO(role);
+
+static ssize_t mechanism_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ int i = get_password_instance_id(kobj);
+
+ if (i < 0)
+ return i;
+
+ if (bioscfg_drv.password_data[i].mechanism != PASSWORD)
+ return -EINVAL;
+
+ return sysfs_emit(buf, "%s\n", PASSWD_MECHANISM_TYPES);
+}
+static struct kobj_attribute password_mechanism = __ATTR_RO(mechanism);
+
+static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "password\n");
+}
+static struct kobj_attribute password_type = __ATTR_RO(type);
+
+ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, password);
+static struct kobj_attribute password_display_name =
+ __ATTR_RO(display_name);
+
+ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, password);
+static struct kobj_attribute password_display_langcode =
+ __ATTR_RO(display_name_language_code);
+
+ATTRIBUTE_N_COMMON_PROPERTY_SHOW(prerequisites_size, password);
+static struct kobj_attribute password_prerequisites_size_val =
+ __ATTR_RO(prerequisites_size);
+
+ATTRIBUTE_V_COMMON_PROPERTY_SHOW(prerequisites, password);
+static struct kobj_attribute password_prerequisites_val =
+ __ATTR_RO(prerequisites);
+
+ATTRIBUTE_N_PROPERTY_SHOW(encodings_size, password);
+static struct kobj_attribute password_encodings_size_val =
+ __ATTR_RO(encodings_size);
+
+ATTRIBUTE_VALUES_PROPERTY_SHOW(encodings, password);
+static struct kobj_attribute password_encodings_val =
+ __ATTR_RO(encodings);
+
+
+static struct attribute *password_attrs[] = {
+ &password_is_password_set.attr,
+ &password_min_password_length.attr,
+ &password_max_password_length.attr,
+ &password_current_password.attr,
+ &password_new_password.attr,
+ &password_role.attr,
+ &password_mechanism.attr,
+ &password_type.attr,
+ &password_display_name.attr,
+ &password_display_langcode.attr,
+ &password_prerequisites_size_val.attr,
+ &password_prerequisites_val.attr,
+ &password_encodings_val.attr,
+ &password_encodings_size_val.attr,
+ NULL
+};
+
+static const struct attribute_group bios_password_attr_group = {
+ .attrs = password_attrs
+};
+
+static const struct attribute_group system_password_attr_group = {
+ .attrs = password_attrs
+};
+
+int alloc_password_data(void)
+{
+ int ret = 0;
+
+ bioscfg_drv.password_instances_count = get_instance_count(HP_WMI_BIOS_PASSWORD_GUID);
+ bioscfg_drv.password_data = kcalloc(bioscfg_drv.password_instances_count,
+ sizeof(struct password_data), GFP_KERNEL);
+ if (!bioscfg_drv.password_data) {
+ bioscfg_drv.password_instances_count = 0;
+ ret = -ENOMEM;
+ }
+
+ return ret;
+}
+
+/*
+ * populate_password_package_data -
+ * Populate all properties for an instance under password attribute
+ *
+ * @password_obj: ACPI object with password data
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ */
+int populate_password_package_data(union acpi_object *password_obj, int instance_id,
+ struct kobject *attr_name_kobj)
+{
+ bioscfg_drv.password_data[instance_id].attr_name_kobj = attr_name_kobj;
+
+ populate_password_elements_from_package(password_obj,
+ password_obj->package.count,
+ instance_id);
+
+ if (strcmp(attr_name_kobj->name, "Setup Password") == 0) {
+ /* Save system authentication instance for easy access */
+ return sysfs_create_group(attr_name_kobj, &bios_password_attr_group);
+ }
+
+ return sysfs_create_group(attr_name_kobj, &system_password_attr_group);
+}
+
+/* Expected Values types associated with each element */
+static const acpi_object_type expected_password_types[] = {
+ [NAME] = ACPI_TYPE_STRING,
+ [VALUE] = ACPI_TYPE_STRING,
+ [PATH] = ACPI_TYPE_STRING,
+ [IS_READONLY] = ACPI_TYPE_INTEGER,
+ [DISPLAY_IN_UI] = ACPI_TYPE_INTEGER,
+ [REQUIRES_PHYSICAL_PRESENCE] = ACPI_TYPE_INTEGER,
+ [SEQUENCE] = ACPI_TYPE_INTEGER,
+ [PREREQUISITES_SIZE] = ACPI_TYPE_INTEGER,
+ [PREREQUISITES] = ACPI_TYPE_STRING,
+ [SECURITY_LEVEL] = ACPI_TYPE_INTEGER,
+ [PSWD_MIN_LENGTH] = ACPI_TYPE_INTEGER,
+ [PSWD_MAX_LENGTH] = ACPI_TYPE_INTEGER,
+ [PSWD_SIZE] = ACPI_TYPE_INTEGER,
+ [PSWD_ENCODINGS] = ACPI_TYPE_STRING,
+ [PSWD_IS_SET] = ACPI_TYPE_INTEGER
+};
+
+
+int populate_password_elements_from_package(union acpi_object *password_obj,
+ int password_obj_count,
+ int instance_id)
+{
+ char *str_value = NULL;
+ int value_len;
+ int ret = 0;
+ u32 size = 0;
+ u32 int_value;
+ int elem = 0;
+ int reqs;
+ int eloc;
+ int pos_values;
+
+
+ if (!password_obj)
+ return -EINVAL;
+
+ strscpy(bioscfg_drv.password_data[instance_id].common.display_name_language_code,
+ LANG_CODE_STR,
+ sizeof(bioscfg_drv.password_data[instance_id].common.display_name_language_code));
+
+ for (elem = 1, eloc = 1; elem < password_obj_count; elem++, eloc++) {
+
+ /* ONLY look at the first PASSWORD_ELEM_CNT elements */
+ if (eloc == PASSWORD_ELEM_CNT)
+ goto exit_package;
+
+ switch (password_obj[elem].type) {
+ case ACPI_TYPE_STRING:
+
+ if (PREREQUISITES != elem && PSWD_ENCODINGS != elem) {
+ ret = convert_hexstr_to_str(password_obj[elem].string.pointer,
+ password_obj[elem].string.length,
+ &str_value, &value_len);
+ if (ret)
+ continue;
+ }
+ break;
+ case ACPI_TYPE_INTEGER:
+ int_value = (u32)password_obj[elem].integer.value;
+ break;
+ default:
+ pr_warn("Unsupported object type [%d]\n", password_obj[elem].type);
+ continue;
+ }
+
+ /* Check that both expected and read object type match */
+ if (expected_password_types[eloc] != password_obj[elem].type) {
+ pr_err("Error expected type %d for elem %d, but got type %d instead\n",
+ expected_password_types[eloc], elem, password_obj[elem].type);
+ return -EIO;
+ }
+
+ /* Assign appropriate element value to corresponding field*/
+ switch (eloc) {
+ case VALUE:
+ break;
+ case PATH:
+ strscpy(bioscfg_drv.password_data[instance_id].common.path, str_value,
+ sizeof(bioscfg_drv.password_data[instance_id].common.path));
+ break;
+ case IS_READONLY:
+ bioscfg_drv.password_data[instance_id].common.is_readonly = int_value;
+ break;
+ case DISPLAY_IN_UI:
+ bioscfg_drv.password_data[instance_id].common.display_in_ui = int_value;
+ break;
+ case REQUIRES_PHYSICAL_PRESENCE:
+ bioscfg_drv.password_data[instance_id].common.requires_physical_presence = int_value;
+ break;
+ case SEQUENCE:
+ bioscfg_drv.password_data[instance_id].common.sequence = int_value;
+ break;
+ case PREREQUISITES_SIZE:
+ bioscfg_drv.password_data[instance_id].common.prerequisites_size = int_value;
+ if (int_value > MAX_PREREQUISITES_SIZE)
+ pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
+ /*
+ * This HACK is needed to keep the expected
+ * element list pointing to the right obj[elem].type
+ * when the size is zero. PREREQUISITES
+ * object is omitted by BIOS when the size is
+ * zero.
+ */
+ if (int_value == 0)
+ eloc++;
+ break;
+ case PREREQUISITES:
+ size = bioscfg_drv.password_data[instance_id].common.prerequisites_size;
+
+ for (reqs = 0; reqs < size; reqs++) {
+ ret = convert_hexstr_to_str(password_obj[elem + reqs].string.pointer,
+ password_obj[elem + reqs].string.length,
+ &str_value, &value_len);
+
+ if (ret)
+ break;
+
+ strscpy(bioscfg_drv.password_data[instance_id].common.prerequisites[reqs],
+ str_value,
+ sizeof(bioscfg_drv.password_data[instance_id].common.prerequisites[reqs]));
+
+ kfree(str_value);
+ }
+ break;
+
+ case SECURITY_LEVEL:
+ bioscfg_drv.password_data[instance_id].common.security_level = int_value;
+ break;
+
+ case PSWD_MIN_LENGTH:
+ bioscfg_drv.password_data[instance_id].min_password_length = int_value;
+ break;
+ case PSWD_MAX_LENGTH:
+ bioscfg_drv.password_data[instance_id].max_password_length = int_value;
+ break;
+ case PSWD_SIZE:
+ bioscfg_drv.password_data[instance_id].encodings_size = int_value;
+ if (int_value > MAX_ENCODINGS_SIZE)
+ pr_warn("Password Encoding size value exceeded the maximum number of elements supported or data may be malformed\n");
+
+ /*
+ * This HACK is needed to keep the expected
+ * element list pointing to the right obj[elem].type
+ * when the size is zero. PSWD_ENCODINGS
+ * object is omitted by BIOS when the size is
+ * zero.
+ */
+ if (int_value == 0)
+ eloc++;
+ break;
+
+ case PSWD_ENCODINGS:
+ size = bioscfg_drv.password_data[instance_id].encodings_size;
+
+ for (pos_values = 0; pos_values < size && pos_values < MAX_ENCODINGS_SIZE; pos_values++) {
+ ret = convert_hexstr_to_str(password_obj[elem + pos_values].string.pointer,
+ password_obj[elem + pos_values].string.length,
+ &str_value, &value_len);
+ if (ret)
+ break;
+
+ strscpy(bioscfg_drv.password_data[instance_id].encodings[pos_values],
+ str_value,
+ sizeof(bioscfg_drv.password_data[instance_id].encodings[pos_values]));
+ kfree(str_value);
+ }
+ break;
+ case PSWD_IS_SET:
+ bioscfg_drv.password_data[instance_id].is_enabled = int_value;
+ break;
+
+ default:
+ pr_warn("Invalid element: %d found in Password attribute or data may be malformed\n", elem);
+ break;
+ }
+ kfree(str_value);
+ }
+
+exit_package:
+ kfree(str_value);
+ return 0;
+}
+
+/*
+ * populate_password_buffer_data -
+ * Populate all properties for an instance under password object attribute
+ *
+ * @buffer_ptr: Buffer pointer
+ * @buffer_size: Buffer size
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ */
+int populate_password_buffer_data(u8 *buffer_ptr, u32 *buffer_size, int instance_id,
+ struct kobject *attr_name_kobj)
+{
+ bioscfg_drv.password_data[instance_id].attr_name_kobj = attr_name_kobj;
+
+ /* Populate Password attributes */
+ populate_password_elements_from_buffer(buffer_ptr, buffer_size,
+ instance_id);
+ friendly_user_name_update(bioscfg_drv.password_data[instance_id].common.path,
+ attr_name_kobj->name,
+ bioscfg_drv.password_data[instance_id].common.display_name,
+ sizeof(bioscfg_drv.password_data[instance_id].common.display_name));
+ if (strcmp(attr_name_kobj->name, "Setup Password") == 0)
+ return sysfs_create_group(attr_name_kobj, &bios_password_attr_group);
+
+ return sysfs_create_group(attr_name_kobj, &system_password_attr_group);
+}
+
+int populate_password_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
+ int instance_id)
+{
+ int ret;
+ char *dst = NULL;
+ int elem;
+ int reqs;
+ int integer;
+ int size = 0;
+ int values;
+ int dst_size = *buffer_size / sizeof(u16);
+
+ dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
+ if (!dst)
+ return -ENOMEM;
+
+ elem = 0;
+ strscpy(bioscfg_drv.password_data[instance_id].common.display_name_language_code,
+ LANG_CODE_STR,
+ sizeof(bioscfg_drv.password_data[instance_id].common.display_name_language_code));
+
+ for (elem = 1; elem < 3; elem++) {
+
+ ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
+ if (ret < 0)
+ continue;
+
+ switch (elem) {
+ case VALUE:
+ strscpy(bioscfg_drv.password_data[instance_id].current_password,
+ dst, sizeof(bioscfg_drv.password_data[instance_id].current_password));
+ break;
+ case PATH:
+ strscpy(bioscfg_drv.password_data[instance_id].common.path, dst,
+ sizeof(bioscfg_drv.password_data[instance_id].common.path));
+ break;
+ default:
+ pr_warn("Invalid element: %d found in Password attribute or data may be malformed\n", elem);
+ break;
+ }
+ }
+
+ for (elem = 3; elem < PASSWORD_ELEM_CNT; elem++) {
+
+ if (elem != PREREQUISITES && elem != PSWD_ENCODINGS) {
+ ret = get_integer_from_buffer((int **)&buffer_ptr, buffer_size, (int *)&integer);
+ if (ret)
+ continue;
+ }
+
+ switch (elem) {
+ case IS_READONLY:
+ bioscfg_drv.password_data[instance_id].common.is_readonly = integer;
+ break;
+ case DISPLAY_IN_UI:
+ bioscfg_drv.password_data[instance_id].common.display_in_ui = integer;
+ break;
+ case REQUIRES_PHYSICAL_PRESENCE:
+ bioscfg_drv.password_data[instance_id].common.requires_physical_presence = integer;
+ break;
+ case SEQUENCE:
+ bioscfg_drv.password_data[instance_id].common.sequence = integer;
+ break;
+ case PREREQUISITES_SIZE:
+ bioscfg_drv.password_data[instance_id].common.prerequisites_size = integer;
+ if (integer > MAX_PREREQUISITES_SIZE)
+ pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
+
+ // PREREQUISITES:
+ elem++;
+ size = bioscfg_drv.password_data[instance_id].common.prerequisites_size;
+ for (reqs = 0; reqs < size && reqs > MAX_PREREQUISITES_SIZE; reqs++) {
+ ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
+ if (ret < 0)
+ continue;
+
+ strscpy(bioscfg_drv.password_data[instance_id].common.prerequisites[reqs],
+ dst,
+ sizeof(bioscfg_drv.password_data[instance_id].common.prerequisites[reqs]));
+ }
+ break;
+ case SECURITY_LEVEL:
+ bioscfg_drv.password_data[instance_id].common.security_level = integer;
+ break;
+
+ case PSWD_MIN_LENGTH:
+ bioscfg_drv.password_data[instance_id].min_password_length = integer;
+ break;
+ case PSWD_MAX_LENGTH:
+ bioscfg_drv.password_data[instance_id].max_password_length = integer;
+ break;
+ case PSWD_SIZE:
+ bioscfg_drv.password_data[instance_id].encodings_size = integer;
+ if (integer > MAX_ENCODINGS_SIZE)
+ pr_warn("Password Encoding size value exceeded the maximum number of elements supported or data may be malformed\n");
+
+ // PSWD_ENCODINGS:
+ elem++;
+ size = bioscfg_drv.password_data[instance_id].encodings_size;
+ for (values = 0; values < size && values < MAX_ENCODINGS_SIZE; values++) {
+ ret = get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
+ if (ret < 0)
+ continue;
+
+ strscpy(bioscfg_drv.password_data[instance_id].encodings[values],
+ dst,
+ sizeof(bioscfg_drv.password_data[instance_id].encodings[values]));
+
+ }
+ break;
+ case PSWD_IS_SET:
+ bioscfg_drv.password_data[instance_id].is_enabled = integer;
+ break;
+ default:
+ pr_warn("Invalid element: %d found in Password attribute or data may be malformed\n", elem);
+ break;
+ }
+ }
+ kfree(dst);
+ return 0;
+}
+
+/*
+ * exit_password_attributes() - Clear all attribute data
+ *
+ * Clears all data allocated for this group of attributes
+ */
+void exit_password_attributes(void)
+{
+ int instance_id;
+
+ for (instance_id = 0; instance_id < bioscfg_drv.password_instances_count; instance_id++) {
+ struct kobject *attr_name_kobj = bioscfg_drv.password_data[instance_id].attr_name_kobj;
+
+ if (attr_name_kobj) {
+ if (strcmp(attr_name_kobj->name, SETUP_PASSWD) == 0)
+ sysfs_remove_group(attr_name_kobj,
+ &bios_password_attr_group);
+ else
+ sysfs_remove_group(attr_name_kobj,
+ &system_password_attr_group);
+ }
+ }
+ bioscfg_drv.password_instances_count = 0;
+ kfree(bioscfg_drv.password_data);
+ bioscfg_drv.password_data = NULL;
+}
--
2.34.1

2023-04-20 16:57:36

by Jorge Lopez

[permalink] [raw]

On Fri, Apr 28, 2023 at 11:30 AM Thomas Weißschuh <[email protected]> wrote:
>
> On 2023-04-28 11:19:04-0500, Jorge Lopez wrote:
> > On Fri, Apr 28, 2023 at 11:09 AM Thomas Weißschuh <[email protected]> wrote:
> > >
> > > On 2023-04-28 11:03:56-0500, Jorge Lopez wrote:
> > > > On Fri, Apr 28, 2023 at 10:36 AM Thomas Weißschuh <[email protected]> wrote:
> > > > >
> > > > > On 2023-04-28 10:24:40-0500, Jorge Lopez wrote:
> > > > > > On Sun, Apr 23, 2023 at 7:01 AM Thomas Weißschuh <[email protected]> wrote:
> > > > > > >
> > > > > > > On 2023-04-20 11:54:48-0500, Jorge Lopez wrote:
> > > > > > > > ---
> > > > > > > > drivers/platform/x86/hp/hp-bioscfg/bioscfg.h | 613 +++++++++++++++++++
> > > > > > > > 1 file changed, 613 insertions(+)
> > > > > > > > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
> > >
> > > <snip>
> > >
> > > > > > > > +#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 = NULL; \
> > > > > > > > + char *attr_value = NULL; \
> > > > > > > > + int i; \
> > > > > > > > + int ret = -EIO; \
> > > > > > > > + \
> > > > > > > > + attr_value = kstrdup(buf, GFP_KERNEL); \
> > > > > > > > + if (!attr_value) \
> > > > > > > > + return -ENOMEM; \
> > > > > > > > + \
> > > > > > > > + p = memchr(attr_value, '\n', count); \
> > > > > > > > + if (p != NULL) \
> > > > > > > > + *p = '\0'; \
> > > > > > >
> > > > > > > This can also truncate the string if there is data after the newline.
> > > > > >
> > > > > > This is a expected behavior as described by Hans in a later email
> > > > >
> > > > > I'm fine with stripping a trailing newline.
> > > > >
> > > > > But this truncates the string at the first newline.
> > > > >
> > > > > "foo\nbar" -> "foo"
> > > > > "\nfoo" -> ""
> > > > >
> > > > All inputs expected by this driver and respectively by BIOS are a
> > > > single line. For this reason, '\n' will cause the string to be
> > > > truncated.
> > > > I propose reporting a warning message indicating that the data entered
> > > > has a '\n' character and will be truncated in addition to failing the
> > > > operation with -EINVAL
> > >
> > > EINVAL sounds good, but a warning is overkill IMO.
> > >
> > > Whoever put in the garbage value will see the error.
> > >
> > > Stripping a trailing newline still seems fine though.
> >
> > So. should the driver return an -EINVAL error or truncate the line,
> > report a warning message, and allow it to proceed.?
> > Please advice
>
> p = memchr(attr_value, '\n', count)
> if (p == attr_value + count - 1) {
> *p = '\0'; /* strip trailing newline */
> count--;
> } else if (p) {
> return -EINVAL; /* enforce single line input */
> }
>
> (untested)
>
> When putting it into a helper you may need to adapt it a bit.
>
> > > This would be a very good candidate for a helper function.

Done! Helper function created.

2023-04-28 20:56:27

by Jorge Lopez

[permalink] [raw]

On Mon, May 8, 2023 at 3:50 PM Thomas Weißschuh <[email protected]> wrote:
>
> On 2023-05-08 08:56:55-0500, Jorge Lopez wrote:
> > On Sat, May 6, 2023 at 12:51 AM Thomas Weißschuh <[email protected]> wrote:
> > >
> > > On 2023-05-05 16:57:59-0500, Jorge Lopez wrote:
> > > > On Fri, May 5, 2023 at 4:11 PM Thomas Weißschuh <[email protected]> wrote:
> > > > >
> > > > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > > > On Sun, Apr 23, 2023 at 1:55 AM <[email protected]> wrote:
> > > > > > >
> > > > > > > On 2023-04-20 11:54:45-0500, Jorge Lopez wrote:
> > > > > > > > .../x86/hp/hp-bioscfg/ordered-attributes.c | 563 ++++++++++++++++++
> > > > > > > > 1 file changed, 563 insertions(+)
> > > > > > > > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..5e5d540f728d
> > > > > > > > --- /dev/null
> >
> > <snip>
> >
> > > > > > > > + elem_found = 0;
> > > > > > > > + while (elem_found < bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > > > > > > > +
> > > > > > > > + value = strsep(&new_values, ",");
> > > > > > >
> > > > > > > The docs say the separator is semicolon.
> > > > > >
> > > > > > BIOS reports the current value using ',' as separator instead of ";".
> > > > > >
> > > > > > ./hp-bioscfg/attributes/UEFI Boot Order/current_value
> > > > > > HDD:M.2:3,HDD:USB:1(Disabled),HDD:M.2:4,HDD:M.2:1,HDD:M.2:2,NETWORK
> > > > > > IPV4:EMBEDDED:1,NETWORK IPV6:EMBEDDED:1,NETWORK
> > > > > > IPV4:EXPANSION:1,NETWORK IPV6:EXPANSION:1
> > > > > >
> > > > > > To avoid having to convert from "," to ";" and vice versa, I will
> > > > > > update the documentation to reflect the use of "'," commas as the
> > > > > > separator
> > > > >
> > > > > The enum data format uses ";". Therefore it makes sense to also stick to
> > > > > ";".
> > > > > The implementation detail of the BIOS using ',' should not matter to
> > > > > users.
> > > >
> > > > The use of ',' does matter because BIOS expects the updated string to
> > > > use commas as separators
> > > > current_value is reported by BIOS using commas. Any changes to the
> > > > order of items in that string needs to use commas.
> > > >
> > > > The difference with enum is the fact the user needs to enter only one
> > > > value out of those possible values available and no separators are
> > > > needed.
> > > > For ordered attributes...
> > > >
> > > > when the current value is "foo,bar,baz". the user provides a string
> > > > which items are ordered differently. i.e. "baz,bar,foo"
> > > > if the new string is using semicolon instead of comma for the
> > > > separator, BIOS will reject the data.
> > >
> > > Of course the BIOS expects the format with comma.
> > >
> > > But the users of this API should not have to care about implementation
> > > details like that.
> > > They want a consistent API. As the ordered-list type is fairly general
> > > it may be promoted to be a general attribute type later.
> > >
> > > If this happens the API can't be changed as that would break users of
> > > hp-bioscfg. So either there would be two APIs for the ordered-list, one
> > > for hp-bioscfg and one for all other drivers, or different attribute
> > > types would use different kinds of separators.
> > >
> > > Was there an issue with the previous replacement logic?
> >
> > No issue. I was anticipating a potential problem/confusion and created
> > a solution. Customers will start using this driver when it becomes
> > part of the upstream code.
> > Users primarily will use Powershell scripts to interact with the
> > driver. The use of powershell will require the user to have the
> > knowledge and convert ';' semicolon to ',' commas prior to submitting
> > the request to the driver. AFIK, the boot order is one of those
> > attributes that is not configured often by the user. Just my opinion.
>
> If this conversion is done by the driver why would the users have to
> care?

The driver currently does not do any conversion hence the reason for
my concerns.
I will reinstate the conversion which was removed in an earlier review.
>
> > >
> > > The whole point of device drivers is to translate general kernel APIs
> > > into whatever a specific device needs, switching around ',' and ';'
> > > seems not so bad.
> >
> > So, are you saying, it is ok for the driver to convert ';' semicolon
> > to ',' commas prior to submitting a request to change any attribute
> > of type 'ordered-list' or when reading current value ?
> > If that is correct, then we can change the documentation back to use
> > ';' semicolons.
> > Please confirm.
>
> A device drivers task it to translate from the domain of the kernel and
> its unified APIs to whatever a specific device needs.
>
> If we agree that it is more consistent to use semicolons for class
> firmware-attribute APIs then the driver would make sure that userspace
> always sees semicolons while the ACPI calls always use commas.
>
> This means translating semicolon->comma when writing to and
> comma->semicolon when reading from ACPI.
> Userspace should not be exposed to the implementation details but just
> the consistent and documented kernel API.
>
> > > > > > > > + if (value != NULL) {
> > > > > > > > + if (!*value)
> > > > > > > > + continue;
> > > > > > > > + elem_found++;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + found = 0;
> > > > > > > > + for (elem = 0; elem < bioscfg_drv.ordered_list_data[instance_id].elements_size; elem++) {
> > > > > > > > + if (!strcasecmp(bioscfg_drv.ordered_list_data[instance_id].elements[elem], value)) {
> > > > > > >
> > > > > > > It's surprising that this is case-insensitive.
> > > > > >
> > > > > > As validated in earlier reviews, BIOS rejects strings that do not
> > > > > > match the internal values.
> > > > > >
> >
> > <snip>
> >
> > > > > > > > +
> > > > > > > > + strscpy(bioscfg_drv.ordered_list_data[instance_id].common.display_name_language_code,
> > > > > > > > + LANG_CODE_STR,
> > > > > > > > + sizeof(bioscfg_drv.ordered_list_data[instance_id].common.display_name_language_code));
> > > > > > >
> > > > > > > This seems to be the same for every type. Can it not be moved into
> > > > > > > common code?
> > > > > >
> > > > > > Each instance requires to report 'display_name_language_code' hence it
> > > > > > cannot be moved to a common code.
> > > > >
> > > > > Can it every be different from LANG_CODE_STR?
> > > >
> > > > Yes. The string currently is LANG_CODE_STR (en_US.UTF-8) but it
> > > > will change as the BIOS provides additional translations at a later
> > > > time.
> > > > This is a future enhancement for the driver.
> > >
> > > Is this planned for the near future?
> > > And/Or already implemented in the BIOS?
> > >
> > > If there are no concrete plans to implement this soon, in my opinion,
> > > it would be better to only introduce this code when it does something
> > > useful.
> >
> > There are no concrete plans yet for the driver. BIOS provides
> > translations for strings (F10) UI but the attributes are reported in
> > English regardless of the language configured.
> > firmware-attributes requires 'display_name_language_code' to be
> > reported. At this time, the driver can provide a common helper to
> > assign the LANG_CODE_STR to the corresponding attribute.
>
> Yes please.
>
> Create an attribute in bioscfg.c that can be used in all the other
> attribute_groups.

Will do!
>
> > > > > If not instead of having one kobj_attribute diplay_langcode for each
> > > > > string/enum/integer/etc you could have one kobj_attribute defined in
> > > > > bioscfg.c and then added to each string_attrs, enum_attrs...
> > > > >
> > > > > The _show() callback for this attribute would just return the constant
> > > > > string.
> > > > >
> > > > > This removes the need for many attribute definition, members in the data
> > > > > struct, initialization of these member...

2023-05-09 18:56:07

by Jorge Lopez

[permalink] [raw]

Subject: Re: [PATCH v11 05/14] HP BIOSCFG driver - ordered-attributes

On Mon, May 8, 2023 at 3:50 PM Thomas Weißschuh <[email protected]> wrote:
>
> On 2023-05-08 08:56:55-0500, Jorge Lopez wrote:
> > On Sat, May 6, 2023 at 12:51 AM Thomas Weißschuh <[email protected]> wrote:
> > >
> > > On 2023-05-05 16:57:59-0500, Jorge Lopez wrote:
> > > > On Fri, May 5, 2023 at 4:11 PM Thomas Weißschuh <[email protected]> wrote:
> > > > >
> > > > > On 2023-05-05 11:09:55-0500, Jorge Lopez wrote:
> > > > > > On Sun, Apr 23, 2023 at 1:55 AM <[email protected]> wrote:
> > > > > > >
> > > > > > > On 2023-04-20 11:54:45-0500, Jorge Lopez wrote:
> > > > > > > > .../x86/hp/hp-bioscfg/ordered-attributes.c | 563 ++++++++++++++++++
> > > > > > > > 1 file changed, 563 insertions(+)
> > > > > > > > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/ordered-attributes.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000000..5e5d540f728d
> > > > > > > > --- /dev/null
> >
> > <snip>
> >
> > > > > > > > + elem_found = 0;
> > > > > > > > + while (elem_found < bioscfg_drv.ordered_list_data[instance_id].elements_size) {
> > > > > > > > +
> > > > > > > > + value = strsep(&new_values, ",");
> > > > > > >
> > > > > > > The docs say the separator is semicolon.
> > > > > >
> > > > > > BIOS reports the current value using ',' as separator instead of ";".
> > > > > >
> > > > > > ./hp-bioscfg/attributes/UEFI Boot Order/current_value
> > > > > > HDD:M.2:3,HDD:USB:1(Disabled),HDD:M.2:4,HDD:M.2:1,HDD:M.2:2,NETWORK
> > > > > > IPV4:EMBEDDED:1,NETWORK IPV6:EMBEDDED:1,NETWORK
> > > > > > IPV4:EXPANSION:1,NETWORK IPV6:EXPANSION:1
> > > > > >
> > > > > > To avoid having to convert from "," to ";" and vice versa, I will
> > > > > > update the documentation to reflect the use of "'," commas as the
> > > > > > separator
> > > > >
> > > > > The enum data format uses ";". Therefore it makes sense to also stick to
> > > > > ";".
> > > > > The implementation detail of the BIOS using ',' should not matter to
> > > > > users.
> > > >
> > > > The use of ',' does matter because BIOS expects the updated string to
> > > > use commas as separators
> > > > current_value is reported by BIOS using commas. Any changes to the
> > > > order of items in that string needs to use commas.
> > > >
> > > > The difference with enum is the fact the user needs to enter only one
> > > > value out of those possible values available and no separators are
> > > > needed.
> > > > For ordered attributes...
> > > >
> > > > when the current value is "foo,bar,baz". the user provides a string
> > > > which items are ordered differently. i.e. "baz,bar,foo"
> > > > if the new string is using semicolon instead of comma for the
> > > > separator, BIOS will reject the data.
> > >
> > > Of course the BIOS expects the format with comma.
> > >
> > > But the users of this API should not have to care about implementation
> > > details like that.
> > > They want a consistent API. As the ordered-list type is fairly general
> > > it may be promoted to be a general attribute type later.
> > >
> > > If this happens the API can't be changed as that would break users of
> > > hp-bioscfg. So either there would be two APIs for the ordered-list, one
> > > for hp-bioscfg and one for all other drivers, or different attribute
> > > types would use different kinds of separators.
> > >
> > > Was there an issue with the previous replacement logic?
> >
> > No issue. I was anticipating a potential problem/confusion and created
> > a solution. Customers will start using this driver when it becomes
> > part of the upstream code.
> > Users primarily will use Powershell scripts to interact with the
> > driver. The use of powershell will require the user to have the
> > knowledge and convert ';' semicolon to ',' commas prior to submitting
> > the request to the driver. AFIK, the boot order is one of those
> > attributes that is not configured often by the user. Just my opinion.
>
> If this conversion is done by the driver why would the users have to
> care?
>
> > >
> > > The whole point of device drivers is to translate general kernel APIs
> > > into whatever a specific device needs, switching around ',' and ';'
> > > seems not so bad.
> >
> > So, are you saying, it is ok for the driver to convert ';' semicolon
> > to ',' commas prior to submitting a request to change any attribute
> > of type 'ordered-list' or when reading current value ?
> > If that is correct, then we can change the documentation back to use
> > ';' semicolons.
> > Please confirm.
>
> A device drivers task it to translate from the domain of the kernel and
> its unified APIs to whatever a specific device needs.
>
> If we agree that it is more consistent to use semicolons for class
> firmware-attribute APIs then the driver would make sure that userspace
> always sees semicolons while the ACPI calls always use commas.
>
> This means translating semicolon->comma when writing to and
> comma->semicolon when reading from ACPI.
> Userspace should not be exposed to the implementation details but just
> the consistent and documented kernel API.

Added conversion routine (replace commas to semicolon and vice versa).
Documentation was updated to reflect that semicolons are used to
separate values.
Userspace is not expose to the implementation details but just the
consistent and documented API

>
> > > > > > > > + if (value != NULL) {
> > > > > > > > + if (!*value)
> > > > > > > > + continue;
> > > > > > > > + elem_found++;
> > > > > > > > + }
> > > > > > > > +
> > > > > > > > + found = 0;
> > > > > > > > + for (elem = 0; elem < bioscfg_drv.ordered_list_data[instance_id].elements_size; elem++) {
> > > > > > > > + if (!strcasecmp(bioscfg_drv.ordered_list_data[instance_id].elements[elem], value)) {
> > > > > > >
> > > > > > > It's surprising that this is case-insensitive.
> > > > > >
> > > > > > As validated in earlier reviews, BIOS rejects strings that do not
> > > > > > match the internal values.
> > > > > >
> >
> > <snip>
> >
> > > > > > > > +
> > > > > > > > + strscpy(bioscfg_drv.ordered_list_data[instance_id].common.display_name_language_code,
> > > > > > > > + LANG_CODE_STR,
> > > > > > > > + sizeof(bioscfg_drv.ordered_list_data[instance_id].common.display_name_language_code));
> > > > > > >
> > > > > > > This seems to be the same for every type. Can it not be moved into
> > > > > > > common code?
> > > > > >
> > > > > > Each instance requires to report 'display_name_language_code' hence it
> > > > > > cannot be moved to a common code.
> > > > >
> > > > > Can it every be different from LANG_CODE_STR?
> > > >
> > > > Yes. The string currently is LANG_CODE_STR (en_US.UTF-8) but it
> > > > will change as the BIOS provides additional translations at a later
> > > > time.
> > > > This is a future enhancement for the driver.
> > >
> > > Is this planned for the near future?
> > > And/Or already implemented in the BIOS?
> > >
> > > If there are no concrete plans to implement this soon, in my opinion,
> > > it would be better to only introduce this code when it does something
> > > useful.
> >
> > There are no concrete plans yet for the driver. BIOS provides
> > translations for strings (F10) UI but the attributes are reported in
> > English regardless of the language configured.
> > firmware-attributes requires 'display_name_language_code' to be
> > reported. At this time, the driver can provide a common helper to
> > assign the LANG_CODE_STR to the corresponding attribute.
>
> Yes please.
>
> Create an attribute in bioscfg.c that can be used in all the other
> attribute_groups.

Done!
>
> > > > > If not instead of having one kobj_attribute diplay_langcode for each
> > > > > string/enum/integer/etc you could have one kobj_attribute defined in
> > > > > bioscfg.c and then added to each string_attrs, enum_attrs...
> > > > >
> > > > > The _show() callback for this attribute would just return the constant
> > > > > string.
> > > > >
> > > > > This removes the need for many attribute definition, members in the data
> > > > > struct, initialization of these member...