[v12,04/13] HP BIOSCFG driver - int-attributes
Commit Message
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 <jorge.lopez2@hp.com>
---
Based on the latest platform-drivers-x86.git/for-next
---
.../x86/hp/hp-bioscfg/int-attributes.c | 448 ++++++++++++++++++
1 file changed, 448 insertions(+)
create mode 100644 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
Comments
On Fri, 5 May 2023, Jorge Lopez wrote:
> 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 <jorge.lopez2@hp.com>
>
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
> .../x86/hp/hp-bioscfg/int-attributes.c | 448 ++++++++++++++++++
> 1 file changed, 448 insertions(+)
> create mode 100644 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
>
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> new file mode 100644
> index 000000000000..1395043d5c9f
> --- /dev/null
> +++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Functions corresponding to integer type attributes under
> + * BIOS Enumeration GUID for use with hp-bioscfg driver.
> + *
> + * Copyright (c) 2022 Hewlett-Packard Inc.
> + */
> +
> +#include "bioscfg.h"
> +
> +GET_INSTANCE_ID(integer);
> +
> +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + int instance_id = get_integer_instance_id(kobj);
> +
> + if (instance_id < 0)
> + return -EIO;
> +
> + return sysfs_emit(buf, "%d\n",
> + bioscfg_drv.integer_data[instance_id].current_value);
> +}
> +
> +/*
> + * 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, char *buf)
> +{
> + int in_val;
> + int ret;
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + /* BIOS treats it as a read only attribute */
> + if (integer_data->common.is_readonly)
> + return -EIO;
> +
> + ret = kstrtoint(buf, 10, &in_val);
> + if (ret < 0)
> + return ret;
> +
> + if (in_val < integer_data->lower_bound ||
> + in_val > integer_data->upper_bound)
> + return -ERANGE;
> +
> + /*
> + * set pending reboot flag depending on
> + * "RequiresPhysicalPresence" value
> + */
> + if (integer_data->common.requires_physical_presence)
> + set_reboot_and_signal_event();
> + return 0;
> +}
> +
> +static void update_integer_value(int instance_id, char *attr_value)
> +{
> + int in_val;
> + int ret;
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + ret = kstrtoint(attr_value, 10, &in_val);
> + if (ret == 0)
> + integer_data->current_value = in_val;
> + else
> + pr_warn("Invalid integer value found: %s\n", attr_value);
> +}
> +
> +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, integer);
> +static struct kobj_attribute integer_display_langcode =
> + __ATTR_RO(display_name_language_code);
> +
> +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, integer);
> +static struct kobj_attribute integer_display_name =
> + __ATTR_RO(display_name);
> +
> +ATTRIBUTE_PROPERTY_STORE(current_value, integer);
> +static struct kobj_attribute integer_current_val =
> + __ATTR_RW_MODE(current_value, 0644);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(lower_bound, integer);
> +static struct kobj_attribute integer_lower_bound =
> + __ATTR_RO(lower_bound);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(upper_bound, integer);
> +static struct kobj_attribute integer_upper_bound =
> + __ATTR_RO(upper_bound);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(scalar_increment, integer);
> +static struct kobj_attribute integer_scalar_increment =
> + __ATTR_RO(scalar_increment);
> +
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "integer\n");
> +}
> +
> +static struct kobj_attribute integer_type =
> + __ATTR_RO(type);
> +
> +static struct attribute *integer_attrs[] = {
> + &integer_display_langcode.attr,
> + &integer_display_name.attr,
> + &integer_current_val.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_integer_data(void)
> +{
> + bioscfg_drv.integer_instances_count = get_instance_count(HP_WMI_BIOS_INTEGER_GUID);
> + bioscfg_drv.integer_data = kcalloc(bioscfg_drv.integer_instances_count,
> + sizeof(struct integer_data), GFP_KERNEL);
It would be better to use sizeof(*...) format.
> +
> + if (!bioscfg_drv.integer_data) {
> + bioscfg_drv.integer_instances_count = 0;
> + return -ENOMEM;
> + }
> + return 0;
> +}
> +
> +/* Expected Values types associated with each element */
> +static const acpi_object_type expected_integer_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,
> + [INT_LOWER_BOUND] = ACPI_TYPE_INTEGER,
> + [INT_UPPER_BOUND] = ACPI_TYPE_INTEGER,
> + [INT_SCALAR_INCREMENT] = ACPI_TYPE_INTEGER,
> +};
> +
> +/*
> + * 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_integer_package_data(union acpi_object *integer_obj,
> + int instance_id,
> + struct kobject *attr_name_kobj)
> +{
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + integer_data->attr_name_kobj = attr_name_kobj;
> + populate_integer_elements_from_package(integer_obj,
> + integer_obj->package.count,
> + instance_id);
> + update_attribute_permissions(integer_data->common.is_readonly,
> + &integer_current_val);
> + friendly_user_name_update(integer_data->common.path,
> + attr_name_kobj->name,
> + integer_data->common.display_name,
> + sizeof(integer_data->common.display_name));
> + return sysfs_create_group(attr_name_kobj, &integer_attr_group);
> +}
> +
> +int populate_integer_elements_from_package(union acpi_object *integer_obj,
> + int integer_obj_count,
> + int instance_id)
> +{
> + char *str_value = NULL;
> + int value_len;
> + int ret;
> + u32 int_value;
> + int elem;
> + int reqs;
> + int eloc;
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + if (!integer_obj)
> + return -EINVAL;
> +
> + strscpy(integer_data->common.display_name_language_code,
> + LANG_CODE_STR,
> + sizeof(integer_data->common.display_name_language_code));
> +
> + for (elem = 1, eloc = 1; elem < integer_obj_count; elem++, eloc++) {
> + /* ONLY look at the first INTEGER_ELEM_CNT elements */
> + if (eloc == INT_ELEM_CNT)
> + goto exit_integer_package;
> +
> + switch (integer_obj[elem].type) {
> + case ACPI_TYPE_STRING:
> +
Extra newline.
> + if (elem != PREREQUISITES) {
> + ret = convert_hexstr_to_str(integer_obj[elem].string.pointer,
> + integer_obj[elem].string.length,
> + &str_value, &value_len);
> + if (ret)
> + continue;
> + }
> + break;
> + case ACPI_TYPE_INTEGER:
> + int_value = (u32)integer_obj[elem].integer.value;
> + break;
> + default:
> + pr_warn("Unsupported object type [%d]\n", integer_obj[elem].type);
> + continue;
> + }
> + /* Check that both expected and read object type match */
> + if (expected_integer_types[eloc] != integer_obj[elem].type) {
> + pr_err("Error expected type %d for elem %d, but got type %d instead\n",
> + expected_integer_types[eloc], elem, integer_obj[elem].type);
> + return -EIO;
> + }
> + /* Assign appropriate element value to corresponding field*/
> + switch (eloc) {
> + case VALUE:
> + ret = kstrtoint(str_value, 10, &int_value);
> + if (ret)
> + continue;
> +
> + integer_data->current_value = int_value;
> + break;
> + case PATH:
> + strscpy(integer_data->common.path, str_value,
> + sizeof(integer_data->common.path));
> + break;
> + case IS_READONLY:
> + integer_data->common.is_readonly = int_value;
> + break;
> + case DISPLAY_IN_UI:
> + integer_data->common.display_in_ui = int_value;
> + break;
> + case REQUIRES_PHYSICAL_PRESENCE:
> + integer_data->common.requires_physical_presence = int_value;
> + break;
> + case SEQUENCE:
> + integer_data->common.sequence = int_value;
> + break;
> + case PREREQUISITES_SIZE:
> + if (integer_data->common.prerequisites_size > 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 (integer_data->common.prerequisites_size == 0)
> + eloc++;
> + break;
> + case PREREQUISITES:
> + for (reqs = 0;
> + reqs < integer_data->common.prerequisites_size &&
> + reqs < MAX_PREREQUISITES_SIZE;
> + reqs++) {
> + if (elem >= integer_obj_count) {
> + pr_err("Error elem-objects package is too small\n");
> + return -EINVAL;
> + }
> +
> + ret = convert_hexstr_to_str(integer_obj[elem + reqs].string.pointer,
> + integer_obj[elem + reqs].string.length,
> + &str_value, &value_len);
> +
> + if (ret)
> + continue;
> +
> + strscpy(integer_data->common.prerequisites[reqs],
> + str_value,
> + sizeof(integer_data->common.prerequisites[reqs]));
> + kfree(str_value);
> + }
> + break;
> +
> + case SECURITY_LEVEL:
> + integer_data->common.security_level = int_value;
> + break;
> + case INT_LOWER_BOUND:
> + integer_data->lower_bound = int_value;
> + break;
> + case INT_UPPER_BOUND:
> + integer_data->upper_bound = int_value;
> + break;
> + case INT_SCALAR_INCREMENT:
> + integer_data->scalar_increment = int_value;
> + break;
> + default:
> + pr_warn("Invalid element: %d found in Integer attribute or data may be malformed\n", elem);
> + break;
> + }
> + }
> +exit_integer_package:
> + kfree(str_value);
> + return 0;
> +}
> +
> +/*
> + * populate_integer_buffer_data() -
> + * Populate all properties of an instance under integer attribute
> + *
> + * @buffer_ptr: Buffer pointer
> + * @buffer_size: Buffer size
> + * @instance_id: The instance to enumerate
> + * @attr_name_kobj: The parent kernel object
> + */
> +int populate_integer_buffer_data(u8 *buffer_ptr, u32 *buffer_size, int instance_id,
> + struct kobject *attr_name_kobj)
> +{
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + integer_data->attr_name_kobj = attr_name_kobj;
> +
> + /* Populate integer elements */
> + populate_integer_elements_from_buffer(buffer_ptr, buffer_size,
> + instance_id);
> + update_attribute_permissions(integer_data->common.is_readonly,
> + &integer_current_val);
> + friendly_user_name_update(integer_data->common.path,
> + attr_name_kobj->name,
> + integer_data->common.display_name,
> + sizeof(integer_data->common.display_name));
> +
> + return sysfs_create_group(attr_name_kobj, &integer_attr_group);
> +}
> +
> +int populate_integer_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
> + int instance_id)
> +{
> + char *dst = NULL;
> + int reqs;
> + int ret;
> + int dst_size = *buffer_size / sizeof(u16);
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
> + if (!dst)
> + return -ENOMEM;
> +
> + strscpy(integer_data->common.display_name_language_code,
> + LANG_CODE_STR,
> + sizeof(integer_data->common.display_name_language_code));
> + /*
> + * Only data relevant to this driver and its functionality is
> + * read. BIOS defines the order in which each * element is
> + * read. Element 0 data is not relevant to this
> + * driver hence it is ignored. For clarity, all element names
> + * (DISPLAY_IN_UI) which defines the order in which is read
> + * and the name matches the variable where the data is stored.
> + */
> +
> + // VALUE:
> + get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> + ret = kstrtoint(dst, 10, &integer_data->current_value);
> + if (ret)
> + pr_warn("Unable to convert string to integer: %s\n", dst);
> +
> + // PATH:
> + get_string_from_buffer(&buffer_ptr, buffer_size, integer_data->common.path,
> + sizeof(integer_data->common.path));
> +
> + // IS_READONLY:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.is_readonly);
> +
> + //DISPLAY_IN_UI:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.display_in_ui);
> +
> + // REQUIRES_PHYSICAL_PRESENCE:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.requires_physical_presence);
> +
> + // SEQUENCE:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.sequence);
> +
> + // PREREQUISITES_SIZE:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.prerequisites_size);
> +
> + if (integer_data->common.prerequisites_size > MAX_PREREQUISITES_SIZE) {
> + /* Report a message and limit prerequisite size to maximum value */
> + pr_warn("Integer Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> + integer_data->common.prerequisites_size = MAX_PREREQUISITES_SIZE;
> + }
> +
> + // PREREQUISITES:
> + for (reqs = 0;
> + reqs < integer_data->common.prerequisites_size && reqs < MAX_PREREQUISITES_SIZE;
Why is the second check necessary, didn't you just above force it
prerequisites_size to never be larger than that???
After removing it, put the whole for () for a single line.
> + reqs++)
> + get_string_from_buffer(&buffer_ptr, buffer_size,
> + integer_data->common.prerequisites[reqs],
> + sizeof(integer_data->common.prerequisites[reqs]));
> +
> + // SECURITY_LEVEL:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.security_level);
> +
> + // INT_LOWER_BOUND:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->lower_bound);
> +
> + // INT_UPPER_BOUND:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->upper_bound);
> +
> + // INT_SCALAR_INCREMENT:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->scalar_increment);
> +
> + kfree(dst);
> + return 0;
> +}
> +
> +/*
> + * exit_integer_attributes() - Clear all attribute data
> + *
> + * Clears all data allocated for this group of attributes
> + */
> +void exit_integer_attributes(void)
> +{
> + int instance_id;
> +
> + for (instance_id = 0; instance_id < bioscfg_drv.integer_instances_count;
> + instance_id++) {
> + struct kobject *attr_name_kobj =
> + bioscfg_drv.integer_data[instance_id].attr_name_kobj;
You could consider shorter variable name for instance_id. IMHO, it add
very little value in the long form over i or id.
> +
> + if (attr_name_kobj)
> + sysfs_remove_group(attr_name_kobj, &integer_attr_group);
> + }
> + bioscfg_drv.integer_instances_count = 0;
> +
> + kfree(bioscfg_drv.integer_data);
> + bioscfg_drv.integer_data = NULL;
> +}
>
On 2023-05-05 17:00:34-0500, Jorge Lopez wrote:
<snip>
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
> .../x86/hp/hp-bioscfg/int-attributes.c | 448 ++++++++++++++++++
> 1 file changed, 448 insertions(+)
> create mode 100644 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
>
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> new file mode 100644
> index 000000000000..1395043d5c9f
> --- /dev/null
> +++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> @@ -0,0 +1,448 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Functions corresponding to integer type attributes under
> + * BIOS Enumeration GUID for use with hp-bioscfg driver.
> + *
> + * Copyright (c) 2022 Hewlett-Packard Inc.
> + */
> +
> +#include "bioscfg.h"
> +
> +GET_INSTANCE_ID(integer);
> +
> +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> +{
> + int instance_id = get_integer_instance_id(kobj);
> +
> + if (instance_id < 0)
> + return -EIO;
> +
> + return sysfs_emit(buf, "%d\n",
> + bioscfg_drv.integer_data[instance_id].current_value);
> +}
> +
> +/*
> + * 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, char *buf)
> +{
> + int in_val;
> + int ret;
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + /* BIOS treats it as a read only attribute */
> + if (integer_data->common.is_readonly)
> + return -EIO;
> +
> + ret = kstrtoint(buf, 10, &in_val);
> + if (ret < 0)
> + return ret;
> +
> + if (in_val < integer_data->lower_bound ||
> + in_val > integer_data->upper_bound)
> + return -ERANGE;
> +
> + /*
> + * set pending reboot flag depending on
> + * "RequiresPhysicalPresence" value
> + */
> + if (integer_data->common.requires_physical_presence)
> + set_reboot_and_signal_event();
> + return 0;
> +}
> +
> +static void update_integer_value(int instance_id, char *attr_value)
> +{
> + int in_val;
> + int ret;
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + ret = kstrtoint(attr_value, 10, &in_val);
> + if (ret == 0)
> + integer_data->current_value = in_val;
> + else
> + pr_warn("Invalid integer value found: %s\n", attr_value);
> +}
> +
> +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, integer);
> +static struct kobj_attribute integer_display_langcode =
> + __ATTR_RO(display_name_language_code);
> +
> +ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, integer);
> +static struct kobj_attribute integer_display_name =
> + __ATTR_RO(display_name);
> +
> +ATTRIBUTE_PROPERTY_STORE(current_value, integer);
> +static struct kobj_attribute integer_current_val =
> + __ATTR_RW_MODE(current_value, 0644);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(lower_bound, integer);
> +static struct kobj_attribute integer_lower_bound =
> + __ATTR_RO(lower_bound);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(upper_bound, integer);
> +static struct kobj_attribute integer_upper_bound =
> + __ATTR_RO(upper_bound);
> +
> +ATTRIBUTE_N_PROPERTY_SHOW(scalar_increment, integer);
> +static struct kobj_attribute integer_scalar_increment =
> + __ATTR_RO(scalar_increment);
> +
> +static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + return sysfs_emit(buf, "integer\n");
> +}
> +
> +static struct kobj_attribute integer_type =
> + __ATTR_RO(type);
> +
> +static struct attribute *integer_attrs[] = {
> + &integer_display_langcode.attr,
> + &integer_display_name.attr,
> + &integer_current_val.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_integer_data(void)
> +{
> + bioscfg_drv.integer_instances_count = get_instance_count(HP_WMI_BIOS_INTEGER_GUID);
> + bioscfg_drv.integer_data = kcalloc(bioscfg_drv.integer_instances_count,
> + sizeof(struct integer_data), GFP_KERNEL);
> +
> + if (!bioscfg_drv.integer_data) {
> + bioscfg_drv.integer_instances_count = 0;
> + return -ENOMEM;
> + }
> + return 0;
> +}
> +
> +/* Expected Values types associated with each element */
> +static const acpi_object_type expected_integer_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,
> + [INT_LOWER_BOUND] = ACPI_TYPE_INTEGER,
> + [INT_UPPER_BOUND] = ACPI_TYPE_INTEGER,
> + [INT_SCALAR_INCREMENT] = ACPI_TYPE_INTEGER,
> +};
> +
> +/*
> + * 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_integer_package_data(union acpi_object *integer_obj,
> + int instance_id,
> + struct kobject *attr_name_kobj)
> +{
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + integer_data->attr_name_kobj = attr_name_kobj;
> + populate_integer_elements_from_package(integer_obj,
> + integer_obj->package.count,
> + instance_id);
> + update_attribute_permissions(integer_data->common.is_readonly,
> + &integer_current_val);
> + friendly_user_name_update(integer_data->common.path,
> + attr_name_kobj->name,
> + integer_data->common.display_name,
> + sizeof(integer_data->common.display_name));
> + return sysfs_create_group(attr_name_kobj, &integer_attr_group);
> +}
> +
> +int populate_integer_elements_from_package(union acpi_object *integer_obj,
> + int integer_obj_count,
> + int instance_id)
> +{
> + char *str_value = NULL;
> + int value_len;
> + int ret;
> + u32 int_value;
> + int elem;
> + int reqs;
> + int eloc;
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + if (!integer_obj)
> + return -EINVAL;
> +
> + strscpy(integer_data->common.display_name_language_code,
> + LANG_CODE_STR,
> + sizeof(integer_data->common.display_name_language_code));
> +
> + for (elem = 1, eloc = 1; elem < integer_obj_count; elem++, eloc++) {
> + /* ONLY look at the first INTEGER_ELEM_CNT elements */
> + if (eloc == INT_ELEM_CNT)
> + goto exit_integer_package;
> +
> + switch (integer_obj[elem].type) {
> + case ACPI_TYPE_STRING:
> +
> + if (elem != PREREQUISITES) {
> + ret = convert_hexstr_to_str(integer_obj[elem].string.pointer,
> + integer_obj[elem].string.length,
> + &str_value, &value_len);
> + if (ret)
> + continue;
> + }
> + break;
Instead of the loop pattern can this not use the same pattern as
populate_integer_elements_from_buffer()?
(Same for all attribute types)
> + case ACPI_TYPE_INTEGER:
> + int_value = (u32)integer_obj[elem].integer.value;
> + break;
> + default:
> + pr_warn("Unsupported object type [%d]\n", integer_obj[elem].type);
> + continue;
> + }
> + /* Check that both expected and read object type match */
> + if (expected_integer_types[eloc] != integer_obj[elem].type) {
> + pr_err("Error expected type %d for elem %d, but got type %d instead\n",
> + expected_integer_types[eloc], elem, integer_obj[elem].type);
> + return -EIO;
> + }
> + /* Assign appropriate element value to corresponding field*/
> + switch (eloc) {
> + case VALUE:
> + ret = kstrtoint(str_value, 10, &int_value);
> + if (ret)
> + continue;
> +
> + integer_data->current_value = int_value;
> + break;
> + case PATH:
> + strscpy(integer_data->common.path, str_value,
> + sizeof(integer_data->common.path));
> + break;
> + case IS_READONLY:
> + integer_data->common.is_readonly = int_value;
> + break;
> + case DISPLAY_IN_UI:
> + integer_data->common.display_in_ui = int_value;
> + break;
> + case REQUIRES_PHYSICAL_PRESENCE:
> + integer_data->common.requires_physical_presence = int_value;
> + break;
> + case SEQUENCE:
> + integer_data->common.sequence = int_value;
> + break;
> + case PREREQUISITES_SIZE:
> + if (integer_data->common.prerequisites_size > 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 (integer_data->common.prerequisites_size == 0)
> + eloc++;
> + break;
> + case PREREQUISITES:
> + for (reqs = 0;
> + reqs < integer_data->common.prerequisites_size &&
> + reqs < MAX_PREREQUISITES_SIZE;
> + reqs++) {
> + if (elem >= integer_obj_count) {
> + pr_err("Error elem-objects package is too small\n");
> + return -EINVAL;
> + }
> +
> + ret = convert_hexstr_to_str(integer_obj[elem + reqs].string.pointer,
> + integer_obj[elem + reqs].string.length,
> + &str_value, &value_len);
> +
> + if (ret)
> + continue;
> +
> + strscpy(integer_data->common.prerequisites[reqs],
> + str_value,
> + sizeof(integer_data->common.prerequisites[reqs]));
> + kfree(str_value);
> + }
> + break;
> +
> + case SECURITY_LEVEL:
> + integer_data->common.security_level = int_value;
> + break;
> + case INT_LOWER_BOUND:
> + integer_data->lower_bound = int_value;
> + break;
> + case INT_UPPER_BOUND:
> + integer_data->upper_bound = int_value;
> + break;
> + case INT_SCALAR_INCREMENT:
> + integer_data->scalar_increment = int_value;
> + break;
> + default:
> + pr_warn("Invalid element: %d found in Integer attribute or data may be malformed\n", elem);
> + break;
> + }
> + }
> +exit_integer_package:
> + kfree(str_value);
> + return 0;
> +}
> +
> +/*
> + * populate_integer_buffer_data() -
> + * Populate all properties of an instance under integer attribute
> + *
> + * @buffer_ptr: Buffer pointer
> + * @buffer_size: Buffer size
> + * @instance_id: The instance to enumerate
> + * @attr_name_kobj: The parent kernel object
> + */
Needs /** to be proper kdoc.
> +int populate_integer_buffer_data(u8 *buffer_ptr, u32 *buffer_size, int instance_id,
> + struct kobject *attr_name_kobj)
> +{
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + integer_data->attr_name_kobj = attr_name_kobj;
> +
> + /* Populate integer elements */
> + populate_integer_elements_from_buffer(buffer_ptr, buffer_size,
> + instance_id);
> + update_attribute_permissions(integer_data->common.is_readonly,
> + &integer_current_val);
> + friendly_user_name_update(integer_data->common.path,
> + attr_name_kobj->name,
> + integer_data->common.display_name,
> + sizeof(integer_data->common.display_name));
> +
> + return sysfs_create_group(attr_name_kobj, &integer_attr_group);
> +}
> +
> +int populate_integer_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
> + int instance_id)
> +{
> + char *dst = NULL;
> + int reqs;
> + int ret;
> + int dst_size = *buffer_size / sizeof(u16);
> + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> +
> + dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
> + if (!dst)
> + return -ENOMEM;
> +
> + strscpy(integer_data->common.display_name_language_code,
> + LANG_CODE_STR,
> + sizeof(integer_data->common.display_name_language_code));
> + /*
> + * Only data relevant to this driver and its functionality is
> + * read. BIOS defines the order in which each * element is
> + * read. Element 0 data is not relevant to this
> + * driver hence it is ignored. For clarity, all element names
> + * (DISPLAY_IN_UI) which defines the order in which is read
> + * and the name matches the variable where the data is stored.
> + */
> +
> + // VALUE:
> + get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> + ret = kstrtoint(dst, 10, &integer_data->current_value);
> + if (ret)
> + pr_warn("Unable to convert string to integer: %s\n", dst);
Maybe set current_value to a well-defined value when this error happens.
> +
> + // PATH:
> + get_string_from_buffer(&buffer_ptr, buffer_size, integer_data->common.path,
> + sizeof(integer_data->common.path));
get_string_from_buffer() returns an int but the return value is never
used.
Also it's not clear where the validation that the buffer is not read out
of bounds happens. Making this more explicit would be nice.
> +
> + // IS_READONLY:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.is_readonly);
> +
> + //DISPLAY_IN_UI:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.display_in_ui);
> +
> + // REQUIRES_PHYSICAL_PRESENCE:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.requires_physical_presence);
> +
> + // SEQUENCE:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.sequence);
> +
> + // PREREQUISITES_SIZE:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.prerequisites_size);
If the common values are always in the same order you could refactor it
into a function.
> +
> + if (integer_data->common.prerequisites_size > MAX_PREREQUISITES_SIZE) {
> + /* Report a message and limit prerequisite size to maximum value */
> + pr_warn("Integer Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> + integer_data->common.prerequisites_size = MAX_PREREQUISITES_SIZE;
> + }
> +
> + // PREREQUISITES:
> + for (reqs = 0;
> + reqs < integer_data->common.prerequisites_size && reqs < MAX_PREREQUISITES_SIZE;
> + reqs++)
> + get_string_from_buffer(&buffer_ptr, buffer_size,
> + integer_data->common.prerequisites[reqs],
> + sizeof(integer_data->common.prerequisites[reqs]));
How is this supposed to work?
Presumably if the firmware returns too many prerequisites the ignored
prerequisites can't just be interpreted as security_level, lower_bound,
upper_bound.
In general it may be useful to be able mark an attribute as invalid and
probihibit interaction from userspace.
Then if the firmware returns bogus data we can just enable that invalid
state and don't have to worry about things like that.
> +
> + // SECURITY_LEVEL:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->common.security_level);
> +
> + // INT_LOWER_BOUND:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->lower_bound);
> +
> + // INT_UPPER_BOUND:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->upper_bound);
> +
> + // INT_SCALAR_INCREMENT:
> + get_integer_from_buffer(&buffer_ptr, buffer_size,
> + &integer_data->scalar_increment);
> +
> + kfree(dst);
dst can be freed much earlier.
> + return 0;
> +}
The new logic looks much nicer!
Now let's see if it can be used for reading from a package.
> +
> +/*
> + * exit_integer_attributes() - Clear all attribute data
> + *
> + * Clears all data allocated for this group of attributes
> + */
> +void exit_integer_attributes(void)
> +{
> + int instance_id;
> +
> + for (instance_id = 0; instance_id < bioscfg_drv.integer_instances_count;
> + instance_id++) {
> + struct kobject *attr_name_kobj =
> + bioscfg_drv.integer_data[instance_id].attr_name_kobj;
> +
> + if (attr_name_kobj)
> + sysfs_remove_group(attr_name_kobj, &integer_attr_group);
> + }
> + bioscfg_drv.integer_instances_count = 0;
> +
> + kfree(bioscfg_drv.integer_data);
> + bioscfg_drv.integer_data = NULL;
> +}
> --
> 2.34.1
>
On Mon, May 8, 2023 at 9:45 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Fri, 5 May 2023, Jorge Lopez wrote:
>
> > 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 <jorge.lopez2@hp.com>
> >
> > ---
> > Based on the latest platform-drivers-x86.git/for-next
> > ---
> > .../x86/hp/hp-bioscfg/int-attributes.c | 448 ++++++++++++++++++
> > 1 file changed, 448 insertions(+)
> > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> >
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > new file mode 100644
> > index 000000000000..1395043d5c9f
> > --- /dev/null
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > @@ -0,0 +1,448 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Functions corresponding to integer type attributes under
> > + * BIOS Enumeration GUID for use with hp-bioscfg driver.
> > + *
> > + * Copyright (c) 2022 Hewlett-Packard Inc.
> > + */
> > +
> > +#include "bioscfg.h"
> > +
> > +GET_INSTANCE_ID(integer);
> > +
> > +static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
> > +{
> > +<snip>
> > +int alloc_integer_data(void)
> > +{
> > + bioscfg_drv.integer_instances_count = get_instance_count(HP_WMI_BIOS_INTEGER_GUID);
> > + bioscfg_drv.integer_data = kcalloc(bioscfg_drv.integer_instances_count,
> > + sizeof(struct integer_data), GFP_KERNEL);
>
> It would be better to use sizeof(*...) format.
I cannot use sizeof(*...) at this time, because it is allocating
bioscfg_drv.integer_instances_count number of integer_data
structures.
>
> > +
> > + if (!bioscfg_drv.integer_data) {
> > + bioscfg_drv.integer_instances_count = 0;
> > + return -ENOMEM;
> > + }
> > + return 0;
> > +}
<snip>
> > +int populate_integer_elements_from_package(union acpi_object *integer_obj,
> > + int integer_obj_count,
> > + int instance_id)
> > +{
> > + char *str_value = NULL;
> > + int value_len;
> > + int ret;
> > + u32 int_value;
> > + int elem;
> > + int reqs;
> > + int eloc;
> > + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> > +
> > + if (!integer_obj)
> > + return -EINVAL;
> > +
> > + strscpy(integer_data->common.display_name_language_code,
> > + LANG_CODE_STR,
> > + sizeof(integer_data->common.display_name_language_code));
> > +
> > + for (elem = 1, eloc = 1; elem < integer_obj_count; elem++, eloc++) {
> > + /* ONLY look at the first INTEGER_ELEM_CNT elements */
> > + if (eloc == INT_ELEM_CNT)
> > + goto exit_integer_package;
> > +
> > + switch (integer_obj[elem].type) {
> > + case ACPI_TYPE_STRING:
> > +
>
> Extra newline.
Done!
>
> > + if (elem != PREREQUISITES) {
> > + ret = convert_hexstr_to_str(integer_obj[elem].string.pointer,
> > + integer_obj[elem].string.length,
> > + &str_value, &value_len);
> > + if (ret)
> > + continue;
> > + }
> > + break;
> > + case ACPI_TYPE_INTEGER:
> > + int_value = (u32)integer_obj[elem].integer.value;
> > + break;
> > + default:
> > + pr_warn("Unsupported object type [%d]\n", integer_obj[elem].type);
> > + continue;
> > + }
> > + /* Check that both expected and read object type match */
<snip>
> > + if (integer_data->common.prerequisites_size > MAX_PREREQUISITES_SIZE) {
> > + /* Report a message and limit prerequisite size to maximum value */
> > + pr_warn("Integer Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> > + integer_data->common.prerequisites_size = MAX_PREREQUISITES_SIZE;
> > + }
> > +
> > + // PREREQUISITES:
> > + for (reqs = 0;
> > + reqs < integer_data->common.prerequisites_size && reqs < MAX_PREREQUISITES_SIZE;
>
> Why is the second check necessary, didn't you just above force it
> prerequisites_size to never be larger than that???
>
> After removing it, put the whole for () for a single line.
I will remove the second check and put the whole () in a single line.
I will apply the same changes to all affected files.
>
> > + reqs++)
> > + get_string_from_buffer(&buffer_ptr, buffer_size,
> > + integer_data->common.prerequisites[reqs],
> > + sizeof(integer_data->common.prerequisites[reqs]));
> > +
> > + // SECURITY_LEVEL:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.security_level);
> > +
> > + // INT_LOWER_BOUND:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->lower_bound);
> > +
> > + // INT_UPPER_BOUND:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->upper_bound);
> > +
> > + // INT_SCALAR_INCREMENT:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->scalar_increment);
> > +
> > + kfree(dst);
> > + return 0;
> > +}
> > +
> > +/*
> > + * exit_integer_attributes() - Clear all attribute data
> > + *
> > + * Clears all data allocated for this group of attributes
> > + */
> > +void exit_integer_attributes(void)
> > +{
> > + int instance_id;
> > +
> > + for (instance_id = 0; instance_id < bioscfg_drv.integer_instances_count;
> > + instance_id++) {
> > + struct kobject *attr_name_kobj =
> > + bioscfg_drv.integer_data[instance_id].attr_name_kobj;
>
> You could consider shorter variable name for instance_id. IMHO, it add
> very little value in the long form over i or id.
>
> > +
> > + if (attr_name_kobj)
> > + sysfs_remove_group(attr_name_kobj, &integer_attr_group);
> > + }
> > + bioscfg_drv.integer_instances_count = 0;
> > +
> > + kfree(bioscfg_drv.integer_data);
> > + bioscfg_drv.integer_data = NULL;
> > +}
> >
>
> --
> i.
<snip>
On Mon, 8 May 2023, Jorge Lopez wrote:
> On Mon, May 8, 2023 at 9:45 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > On Fri, 5 May 2023, Jorge Lopez wrote:
> >
> > > 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 <jorge.lopez2@hp.com>
> > >
> > > ---
> > > Based on the latest platform-drivers-x86.git/for-next
> > > ---
> > > +int alloc_integer_data(void)
> > > +{
> > > + bioscfg_drv.integer_instances_count = get_instance_count(HP_WMI_BIOS_INTEGER_GUID);
> > > + bioscfg_drv.integer_data = kcalloc(bioscfg_drv.integer_instances_count,
> > > + sizeof(struct integer_data), GFP_KERNEL);
> >
> > It would be better to use sizeof(*...) format.
>
> I cannot use sizeof(*...) at this time, because it is allocating
> bioscfg_drv.integer_instances_count number of integer_data
> structures.
Isn't bioscfg_drv.integer_data is a pointer to a single struct
integer_data? Why cannot you do sizeof(*bioscfg_drv.integer_data)?? It is
perfectly legal C construct for getting the size of the struct the
pointer points to.
On Mon, May 8, 2023 at 4:16 PM Thomas Weißschuh <thomas@t-8ch.de> wrote:
>
> On 2023-05-05 17:00:34-0500, Jorge Lopez wrote:
>
> <snip>
>
> > ---
> > Based on the latest platform-drivers-x86.git/for-next
> > ---
> > .../x86/hp/hp-bioscfg/int-attributes.c | 448 ++++++++++++++++++
> > 1 file changed, 448 insertions(+)
> > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> >
> > diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > new file mode 100644
> > index 000000000000..1395043d5c9f
> > --- /dev/null
> > +++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> > @@ -0,0 +1,448 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Functions corresponding to integer type attributes under
> > + * BIOS Enumeration GUID for use with hp-bioscfg driver.
> > + *
> > + * Copyright (c) 2022 Hewlett-Packard Inc.
> > + */
> > +
> > +#include "bioscfg.h"
> > +
> > +GET_INSTANCE_ID(integer);
> > +
<snp>
> > +
> > + for (elem = 1, eloc = 1; elem < integer_obj_count; elem++, eloc++) {
> > + /* ONLY look at the first INTEGER_ELEM_CNT elements */
> > + if (eloc == INT_ELEM_CNT)
> > + goto exit_integer_package;
> > +
> > + switch (integer_obj[elem].type) {
> > + case ACPI_TYPE_STRING:
> > +
> > + if (elem != PREREQUISITES) {
> > + ret = convert_hexstr_to_str(integer_obj[elem].string.pointer,
> > + integer_obj[elem].string.length,
> > + &str_value, &value_len);
> > + if (ret)
> > + continue;
> > + }
> > + break;
>
> Instead of the loop pattern can this not use the same pattern as
> populate_integer_elements_from_buffer()?
>
> (Same for all attribute types)
package data requires keeping track of what is the current package
element and the location between the element.
For these reasons, looping is a better option since the elem counter
needs to be increased.
Another reason is keeping track of the maximum number of elements in
each WMI package so looping provides a cleaner solution.
Let me see how I can clean up the code and make it more readable.
Overall refactoring this portion of the driver can be done at a later
time.
>
> > + case ACPI_TYPE_INTEGER:
> > + int_value = (u32)integer_obj[elem].integer.value;
> > + break;
> > + default:
> > + pr_warn("Unsupported object type [%d]\n", integer_obj[elem].type);
> > + continue;
<snip>
> +
> > +/*
> > + * populate_integer_buffer_data() -
> > + * Populate all properties of an instance under integer attribute
> > + *
> > + * @buffer_ptr: Buffer pointer
> > + * @buffer_size: Buffer size
> > + * @instance_id: The instance to enumerate
> > + * @attr_name_kobj: The parent kernel object
> > + */
>
> Needs /** to be proper kdoc.
Done!
>
> > +int populate_integer_buffer_data(u8 *buffer_ptr, u32 *buffer_size, int instance_id,
> > + struct kobject *attr_name_kobj)
> > +{
> > + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> > +
> > + integer_data->attr_name_kobj = attr_name_kobj;
> > +
> > + /* Populate integer elements */
> > + populate_integer_elements_from_buffer(buffer_ptr, buffer_size,
> > + instance_id);
> > + update_attribute_permissions(integer_data->common.is_readonly,
> > + &integer_current_val);
> > + friendly_user_name_update(integer_data->common.path,
> > + attr_name_kobj->name,
> > + integer_data->common.display_name,
> > + sizeof(integer_data->common.display_name));
> > +
> > + return sysfs_create_group(attr_name_kobj, &integer_attr_group);
> > +}
> > +
> > +int populate_integer_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
> > + int instance_id)
> > +{
> > + char *dst = NULL;
> > + int reqs;
> > + int ret;
> > + int dst_size = *buffer_size / sizeof(u16);
> > + struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
> > +
> > + dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
> > + if (!dst)
> > + return -ENOMEM;
> > +
> > + strscpy(integer_data->common.display_name_language_code,
> > + LANG_CODE_STR,
> > + sizeof(integer_data->common.display_name_language_code));
> > + /*
> > + * Only data relevant to this driver and its functionality is
> > + * read. BIOS defines the order in which each * element is
> > + * read. Element 0 data is not relevant to this
> > + * driver hence it is ignored. For clarity, all element names
> > + * (DISPLAY_IN_UI) which defines the order in which is read
> > + * and the name matches the variable where the data is stored.
> > + */
> > +
> > + // VALUE:
> > + get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
> > + ret = kstrtoint(dst, 10, &integer_data->current_value);
> > + if (ret)
> > + pr_warn("Unable to convert string to integer: %s\n", dst);
>
> Maybe set current_value to a well-defined value when this error happens.
We will initialize the value to zero.
>
> > +
> > + // PATH:
> > + get_string_from_buffer(&buffer_ptr, buffer_size, integer_data->common.path,
> > + sizeof(integer_data->common.path));
>
> get_string_from_buffer() returns an int but the return value is never
> used.
> Also it's not clear where the validation that the buffer is not read out
> of bounds happens. Making this more explicit would be nice.
>
In earlier implementation, errors were ignored and the process
continued to read the next element.
it is for this reason, the return value is not checked since the error
was treated as noop.
I will add a comment explaining why no return value is checked here.
> > +
> > + // IS_READONLY:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.is_readonly);
> > +
> > + //DISPLAY_IN_UI:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.display_in_ui);
> > +
> > + // REQUIRES_PHYSICAL_PRESENCE:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.requires_physical_presence);
> > +
> > + // SEQUENCE:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.sequence);
> > +
> > + // PREREQUISITES_SIZE:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.prerequisites_size);
>
> If the common values are always in the same order you could refactor it
> into a function.
>
> > +
> > + if (integer_data->common.prerequisites_size > MAX_PREREQUISITES_SIZE) {
> > + /* Report a message and limit prerequisite size to maximum value */
> > + pr_warn("Integer Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> > + integer_data->common.prerequisites_size = MAX_PREREQUISITES_SIZE;
> > + }
> > +
> > + // PREREQUISITES:
> > + for (reqs = 0;
> > + reqs < integer_data->common.prerequisites_size && reqs < MAX_PREREQUISITES_SIZE;
> > + reqs++)
> > + get_string_from_buffer(&buffer_ptr, buffer_size,
> > + integer_data->common.prerequisites[reqs],
> > + sizeof(integer_data->common.prerequisites[reqs]));
>
> How is this supposed to work?
>
> Presumably if the firmware returns too many prerequisites the ignored
> prerequisites can't just be interpreted as security_level, lower_bound,
> upper_bound.
>
prerequisites data is considered as corrupted when its prerequisite
size is greater than MAX_PREREQUISITES_SIZE.
Only when the prerequisite size is within range, the driver can assume
the data is valid.
> In general it may be useful to be able mark an attribute as invalid and
> probihibit interaction from userspace.
> Then if the firmware returns bogus data we can just enable that invalid
> state and don't have to worry about things like that.
Good idea. I will keep it in mind.
>
> > +
> > + // SECURITY_LEVEL:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->common.security_level);
> > +
> > + // INT_LOWER_BOUND:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->lower_bound);
> > +
> > + // INT_UPPER_BOUND:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->upper_bound);
> > +
> > + // INT_SCALAR_INCREMENT:
> > + get_integer_from_buffer(&buffer_ptr, buffer_size,
> > + &integer_data->scalar_increment);
> > +
> > + kfree(dst);
>
> dst can be freed much earlier.
Done!
>
> > + return 0;
> > +}
>
> The new logic looks much nicer!
> Now let's see if it can be used for reading from a package.
Please see earlier comment regarding refactoring package data function
and the rationale why there is no significant gain by doing it at this
time.
>
> > +
> > +/*
> > + * exit_integer_attributes() - Clear all attribute data
> > + *
> > + * Clears all data allocated for this group of attributes
> > + */
> > +void exit_integer_attributes(void)
> > +{
> > + int instance_id;
> > +
> > + for (instance_id = 0; instance_id < bioscfg_drv.integer_instances_count;
> > + instance_id++) {
> > + struct kobject *attr_name_kobj =
> > + bioscfg_drv.integer_data[instance_id].attr_name_kobj;
> > +
> > + if (attr_name_kobj)
> > + sysfs_remove_group(attr_name_kobj, &integer_attr_group);
> > + }
> > + bioscfg_drv.integer_instances_count = 0;
> > +
> > + kfree(bioscfg_drv.integer_data);
> > + bioscfg_drv.integer_data = NULL;
> > +}
> > --
> > 2.34.1
> >
On Tue, May 9, 2023 at 5:24 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> On Mon, 8 May 2023, Jorge Lopez wrote:
>
> > On Mon, May 8, 2023 at 9:45 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > On Fri, 5 May 2023, Jorge Lopez wrote:
> > >
> > > > 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 <jorge.lopez2@hp.com>
> > > >
> > > > ---
> > > > Based on the latest platform-drivers-x86.git/for-next
> > > > ---
>
> > > > +int alloc_integer_data(void)
> > > > +{
> > > > + bioscfg_drv.integer_instances_count = get_instance_count(HP_WMI_BIOS_INTEGER_GUID);
> > > > + bioscfg_drv.integer_data = kcalloc(bioscfg_drv.integer_instances_count,
> > > > + sizeof(struct integer_data), GFP_KERNEL);
> > >
> > > It would be better to use sizeof(*...) format.
> >
> > I cannot use sizeof(*...) at this time, because it is allocating
> > bioscfg_drv.integer_instances_count number of integer_data
> > structures.
>
> Isn't bioscfg_drv.integer_data is a pointer to a single struct
> integer_data? Why cannot you do sizeof(*bioscfg_drv.integer_data)?? It is
> perfectly legal C construct for getting the size of the struct the
> pointer points to.
>
>
You are correct. I found the problem in the code during my testing.
I will update similar code across all files.
> --
> i.
new file mode 100644
@@ -0,0 +1,448 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Functions corresponding to integer type attributes under
+ * BIOS Enumeration GUID for use with hp-bioscfg driver.
+ *
+ * Copyright (c) 2022 Hewlett-Packard Inc.
+ */
+
+#include "bioscfg.h"
+
+GET_INSTANCE_ID(integer);
+
+static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
+{
+ int instance_id = get_integer_instance_id(kobj);
+
+ if (instance_id < 0)
+ return -EIO;
+
+ return sysfs_emit(buf, "%d\n",
+ bioscfg_drv.integer_data[instance_id].current_value);
+}
+
+/*
+ * 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, char *buf)
+{
+ int in_val;
+ int ret;
+ struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
+
+ /* BIOS treats it as a read only attribute */
+ if (integer_data->common.is_readonly)
+ return -EIO;
+
+ ret = kstrtoint(buf, 10, &in_val);
+ if (ret < 0)
+ return ret;
+
+ if (in_val < integer_data->lower_bound ||
+ in_val > integer_data->upper_bound)
+ return -ERANGE;
+
+ /*
+ * set pending reboot flag depending on
+ * "RequiresPhysicalPresence" value
+ */
+ if (integer_data->common.requires_physical_presence)
+ set_reboot_and_signal_event();
+ return 0;
+}
+
+static void update_integer_value(int instance_id, char *attr_value)
+{
+ int in_val;
+ int ret;
+ struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
+
+ ret = kstrtoint(attr_value, 10, &in_val);
+ if (ret == 0)
+ integer_data->current_value = in_val;
+ else
+ pr_warn("Invalid integer value found: %s\n", attr_value);
+}
+
+ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name_language_code, integer);
+static struct kobj_attribute integer_display_langcode =
+ __ATTR_RO(display_name_language_code);
+
+ATTRIBUTE_S_COMMON_PROPERTY_SHOW(display_name, integer);
+static struct kobj_attribute integer_display_name =
+ __ATTR_RO(display_name);
+
+ATTRIBUTE_PROPERTY_STORE(current_value, integer);
+static struct kobj_attribute integer_current_val =
+ __ATTR_RW_MODE(current_value, 0644);
+
+ATTRIBUTE_N_PROPERTY_SHOW(lower_bound, integer);
+static struct kobj_attribute integer_lower_bound =
+ __ATTR_RO(lower_bound);
+
+ATTRIBUTE_N_PROPERTY_SHOW(upper_bound, integer);
+static struct kobj_attribute integer_upper_bound =
+ __ATTR_RO(upper_bound);
+
+ATTRIBUTE_N_PROPERTY_SHOW(scalar_increment, integer);
+static struct kobj_attribute integer_scalar_increment =
+ __ATTR_RO(scalar_increment);
+
+static ssize_t type_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ return sysfs_emit(buf, "integer\n");
+}
+
+static struct kobj_attribute integer_type =
+ __ATTR_RO(type);
+
+static struct attribute *integer_attrs[] = {
+ &integer_display_langcode.attr,
+ &integer_display_name.attr,
+ &integer_current_val.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_integer_data(void)
+{
+ bioscfg_drv.integer_instances_count = get_instance_count(HP_WMI_BIOS_INTEGER_GUID);
+ bioscfg_drv.integer_data = kcalloc(bioscfg_drv.integer_instances_count,
+ sizeof(struct integer_data), GFP_KERNEL);
+
+ if (!bioscfg_drv.integer_data) {
+ bioscfg_drv.integer_instances_count = 0;
+ return -ENOMEM;
+ }
+ return 0;
+}
+
+/* Expected Values types associated with each element */
+static const acpi_object_type expected_integer_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,
+ [INT_LOWER_BOUND] = ACPI_TYPE_INTEGER,
+ [INT_UPPER_BOUND] = ACPI_TYPE_INTEGER,
+ [INT_SCALAR_INCREMENT] = ACPI_TYPE_INTEGER,
+};
+
+/*
+ * 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_integer_package_data(union acpi_object *integer_obj,
+ int instance_id,
+ struct kobject *attr_name_kobj)
+{
+ struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
+
+ integer_data->attr_name_kobj = attr_name_kobj;
+ populate_integer_elements_from_package(integer_obj,
+ integer_obj->package.count,
+ instance_id);
+ update_attribute_permissions(integer_data->common.is_readonly,
+ &integer_current_val);
+ friendly_user_name_update(integer_data->common.path,
+ attr_name_kobj->name,
+ integer_data->common.display_name,
+ sizeof(integer_data->common.display_name));
+ return sysfs_create_group(attr_name_kobj, &integer_attr_group);
+}
+
+int populate_integer_elements_from_package(union acpi_object *integer_obj,
+ int integer_obj_count,
+ int instance_id)
+{
+ char *str_value = NULL;
+ int value_len;
+ int ret;
+ u32 int_value;
+ int elem;
+ int reqs;
+ int eloc;
+ struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
+
+ if (!integer_obj)
+ return -EINVAL;
+
+ strscpy(integer_data->common.display_name_language_code,
+ LANG_CODE_STR,
+ sizeof(integer_data->common.display_name_language_code));
+
+ for (elem = 1, eloc = 1; elem < integer_obj_count; elem++, eloc++) {
+ /* ONLY look at the first INTEGER_ELEM_CNT elements */
+ if (eloc == INT_ELEM_CNT)
+ goto exit_integer_package;
+
+ switch (integer_obj[elem].type) {
+ case ACPI_TYPE_STRING:
+
+ if (elem != PREREQUISITES) {
+ ret = convert_hexstr_to_str(integer_obj[elem].string.pointer,
+ integer_obj[elem].string.length,
+ &str_value, &value_len);
+ if (ret)
+ continue;
+ }
+ break;
+ case ACPI_TYPE_INTEGER:
+ int_value = (u32)integer_obj[elem].integer.value;
+ break;
+ default:
+ pr_warn("Unsupported object type [%d]\n", integer_obj[elem].type);
+ continue;
+ }
+ /* Check that both expected and read object type match */
+ if (expected_integer_types[eloc] != integer_obj[elem].type) {
+ pr_err("Error expected type %d for elem %d, but got type %d instead\n",
+ expected_integer_types[eloc], elem, integer_obj[elem].type);
+ return -EIO;
+ }
+ /* Assign appropriate element value to corresponding field*/
+ switch (eloc) {
+ case VALUE:
+ ret = kstrtoint(str_value, 10, &int_value);
+ if (ret)
+ continue;
+
+ integer_data->current_value = int_value;
+ break;
+ case PATH:
+ strscpy(integer_data->common.path, str_value,
+ sizeof(integer_data->common.path));
+ break;
+ case IS_READONLY:
+ integer_data->common.is_readonly = int_value;
+ break;
+ case DISPLAY_IN_UI:
+ integer_data->common.display_in_ui = int_value;
+ break;
+ case REQUIRES_PHYSICAL_PRESENCE:
+ integer_data->common.requires_physical_presence = int_value;
+ break;
+ case SEQUENCE:
+ integer_data->common.sequence = int_value;
+ break;
+ case PREREQUISITES_SIZE:
+ if (integer_data->common.prerequisites_size > 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 (integer_data->common.prerequisites_size == 0)
+ eloc++;
+ break;
+ case PREREQUISITES:
+ for (reqs = 0;
+ reqs < integer_data->common.prerequisites_size &&
+ reqs < MAX_PREREQUISITES_SIZE;
+ reqs++) {
+ if (elem >= integer_obj_count) {
+ pr_err("Error elem-objects package is too small\n");
+ return -EINVAL;
+ }
+
+ ret = convert_hexstr_to_str(integer_obj[elem + reqs].string.pointer,
+ integer_obj[elem + reqs].string.length,
+ &str_value, &value_len);
+
+ if (ret)
+ continue;
+
+ strscpy(integer_data->common.prerequisites[reqs],
+ str_value,
+ sizeof(integer_data->common.prerequisites[reqs]));
+ kfree(str_value);
+ }
+ break;
+
+ case SECURITY_LEVEL:
+ integer_data->common.security_level = int_value;
+ break;
+ case INT_LOWER_BOUND:
+ integer_data->lower_bound = int_value;
+ break;
+ case INT_UPPER_BOUND:
+ integer_data->upper_bound = int_value;
+ break;
+ case INT_SCALAR_INCREMENT:
+ integer_data->scalar_increment = int_value;
+ break;
+ default:
+ pr_warn("Invalid element: %d found in Integer attribute or data may be malformed\n", elem);
+ break;
+ }
+ }
+exit_integer_package:
+ kfree(str_value);
+ return 0;
+}
+
+/*
+ * populate_integer_buffer_data() -
+ * Populate all properties of an instance under integer attribute
+ *
+ * @buffer_ptr: Buffer pointer
+ * @buffer_size: Buffer size
+ * @instance_id: The instance to enumerate
+ * @attr_name_kobj: The parent kernel object
+ */
+int populate_integer_buffer_data(u8 *buffer_ptr, u32 *buffer_size, int instance_id,
+ struct kobject *attr_name_kobj)
+{
+ struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
+
+ integer_data->attr_name_kobj = attr_name_kobj;
+
+ /* Populate integer elements */
+ populate_integer_elements_from_buffer(buffer_ptr, buffer_size,
+ instance_id);
+ update_attribute_permissions(integer_data->common.is_readonly,
+ &integer_current_val);
+ friendly_user_name_update(integer_data->common.path,
+ attr_name_kobj->name,
+ integer_data->common.display_name,
+ sizeof(integer_data->common.display_name));
+
+ return sysfs_create_group(attr_name_kobj, &integer_attr_group);
+}
+
+int populate_integer_elements_from_buffer(u8 *buffer_ptr, u32 *buffer_size,
+ int instance_id)
+{
+ char *dst = NULL;
+ int reqs;
+ int ret;
+ int dst_size = *buffer_size / sizeof(u16);
+ struct integer_data *integer_data = &bioscfg_drv.integer_data[instance_id];
+
+ dst = kcalloc(dst_size, sizeof(char), GFP_KERNEL);
+ if (!dst)
+ return -ENOMEM;
+
+ strscpy(integer_data->common.display_name_language_code,
+ LANG_CODE_STR,
+ sizeof(integer_data->common.display_name_language_code));
+ /*
+ * Only data relevant to this driver and its functionality is
+ * read. BIOS defines the order in which each * element is
+ * read. Element 0 data is not relevant to this
+ * driver hence it is ignored. For clarity, all element names
+ * (DISPLAY_IN_UI) which defines the order in which is read
+ * and the name matches the variable where the data is stored.
+ */
+
+ // VALUE:
+ get_string_from_buffer(&buffer_ptr, buffer_size, dst, dst_size);
+ ret = kstrtoint(dst, 10, &integer_data->current_value);
+ if (ret)
+ pr_warn("Unable to convert string to integer: %s\n", dst);
+
+ // PATH:
+ get_string_from_buffer(&buffer_ptr, buffer_size, integer_data->common.path,
+ sizeof(integer_data->common.path));
+
+ // IS_READONLY:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->common.is_readonly);
+
+ //DISPLAY_IN_UI:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->common.display_in_ui);
+
+ // REQUIRES_PHYSICAL_PRESENCE:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->common.requires_physical_presence);
+
+ // SEQUENCE:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->common.sequence);
+
+ // PREREQUISITES_SIZE:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->common.prerequisites_size);
+
+ if (integer_data->common.prerequisites_size > MAX_PREREQUISITES_SIZE) {
+ /* Report a message and limit prerequisite size to maximum value */
+ pr_warn("Integer Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
+ integer_data->common.prerequisites_size = MAX_PREREQUISITES_SIZE;
+ }
+
+ // PREREQUISITES:
+ for (reqs = 0;
+ reqs < integer_data->common.prerequisites_size && reqs < MAX_PREREQUISITES_SIZE;
+ reqs++)
+ get_string_from_buffer(&buffer_ptr, buffer_size,
+ integer_data->common.prerequisites[reqs],
+ sizeof(integer_data->common.prerequisites[reqs]));
+
+ // SECURITY_LEVEL:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->common.security_level);
+
+ // INT_LOWER_BOUND:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->lower_bound);
+
+ // INT_UPPER_BOUND:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->upper_bound);
+
+ // INT_SCALAR_INCREMENT:
+ get_integer_from_buffer(&buffer_ptr, buffer_size,
+ &integer_data->scalar_increment);
+
+ kfree(dst);
+ return 0;
+}
+
+/*
+ * exit_integer_attributes() - Clear all attribute data
+ *
+ * Clears all data allocated for this group of attributes
+ */
+void exit_integer_attributes(void)
+{
+ int instance_id;
+
+ for (instance_id = 0; instance_id < bioscfg_drv.integer_instances_count;
+ instance_id++) {
+ struct kobject *attr_name_kobj =
+ bioscfg_drv.integer_data[instance_id].attr_name_kobj;
+
+ if (attr_name_kobj)
+ sysfs_remove_group(attr_name_kobj, &integer_attr_group);
+ }
+ bioscfg_drv.integer_instances_count = 0;
+
+ kfree(bioscfg_drv.integer_data);
+ bioscfg_drv.integer_data = NULL;
+}