| Message ID | 20230420165454.9517-3-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | HP BIOSCFG driver | expand |
Hi Jorge, checkpatch.pl finds some issues on your patches. Please make sure checkpath.pl --strict is happy. On 2023-04-20 11:54:42-0500, Jorge Lopez wrote: > --- > Based on the latest platform-drivers-x86.git/for-next > --- > .../x86/hp/hp-bioscfg/biosattr-interface.c | 307 ++++++++++++++++++ > 1 file changed, 307 insertions(+) > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c > new file mode 100644 > index 000000000000..f09dd41867f7 > --- /dev/null > +++ b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c > @@ -0,0 +1,307 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Functions corresponding to methods under BIOS interface GUID > + * for use with hp-bioscfg driver. > + * > + * Copyright (c) 2022 Hewlett-Packard Inc. > + */ > + > +#include <linux/wmi.h> > +#include "bioscfg.h" > + > +#define SET_DEFAULT_VALUES_METHOD_ID 0x02 > +#define SET_BIOS_DEFAULTS_METHOD_ID 0x03 > +#define SET_ATTRIBUTE_METHOD_ID 0x04 This could be an enum. > + > +/* > + * set_attribute() - Update an attribute value > + * @a_name: The attribute name > + * @a_value: The attribute value > + * > + * Sets an attribute to new value > + */ > +int hp_set_attribute(const char *a_name, const char *a_value) > +{ > + size_t security_area_size; > + size_t a_name_size, a_value_size; > + u16 *buffer = NULL; > + u16 *start = NULL; > + int buffer_size; > + int ret = 0; > + int instance; > + char *auth_empty_value = ""; > + char *auth_token_choice = NULL; No need to initialize auth_token_choice and start. Consider coalescing variable declaration to avoid wasting vertical space. > + > + > + mutex_lock(&bioscfg_drv.mutex); > + if (!bioscfg_drv.bios_attr_wdev) { > + ret = -ENODEV; > + goto out_set_attribute; > + } > + > + instance = get_password_instance_for_type(SETUP_PASSWD); > + if (instance < 0) { > + ret = -EINVAL; > + goto out_set_attribute; > + } > + > + if (strlen(bioscfg_drv.password_data[instance].current_password) == 0) > + strscpy(bioscfg_drv.password_data[instance].current_password, > + auth_empty_value, > + sizeof(bioscfg_drv.password_data[instance].current_password)); This essentially does if (current_password[0] == '\0') current_password[0] = '\0'; ... nothing. In the driver there is a lot of dereferencing substructures of bioscfg_drv going on. This makes the code harder to read. > + > + /* Select which auth token to use; password or [auth token] */ > + > + if (bioscfg_drv.spm_data.auth_token != NULL) > + auth_token_choice = bioscfg_drv.spm_data.auth_token; > + else > + auth_token_choice = bioscfg_drv.password_data[instance].current_password; > + > + a_name_size = bioscfg_calculate_string_buffer(a_name); > + a_value_size = bioscfg_calculate_string_buffer(a_value); > + security_area_size = calculate_security_buffer(auth_token_choice); > + buffer_size = a_name_size + a_value_size + security_area_size; > + > + buffer = kmalloc(buffer_size + 1, GFP_KERNEL); > + if (!buffer) { > + ret = -ENOMEM; > + goto out_set_attribute; > + } > + > + /* build variables to set */ > + start = buffer; > + start = ascii_to_utf16_unicode(start, a_name); > + if (!start) > + goto out_set_attribute; ret is 0 here. Is this success? > + > + start = ascii_to_utf16_unicode(start, a_value); > + if (!start) > + goto out_set_attribute; Same as above. > + > + populate_security_buffer(start, auth_token_choice); > + > + ret = hp_wmi_set_bios_setting(buffer, buffer_size); > + > + > +out_set_attribute: > + kfree(buffer); > + mutex_unlock(&bioscfg_drv.mutex); > + return ret; > +} > + > +/* > + * hp_wmi_perform_query > + * > + * query: The commandtype (enum hp_wmi_commandtype) > + * write: The command (enum hp_wmi_command) > + * buffer: Buffer used as input and/or output > + * insize: Size of input buffer > + * outsize: Size of output buffer > + * > + * returns zero on success > + * an HP WMI query specific error code (which is positive) > + * -EINVAL if the query was not successful at all > + * -EINVAL if the output buffer size exceeds buffersize How is the caller supposed to distinguish those? > + * > + * Note: The buffersize must at least be the maximum of the input and output > + * size. E.g. Battery info query is defined to have 1 byte input > + * and 128 byte output. The caller would do: > + * buffer = kzalloc(128, GFP_KERNEL); > + * ret = hp_wmi_perform_query(HPWMI_BATTERY_QUERY, HPWMI_READ, > + * buffer, 1, 128) > + */ > +int hp_wmi_perform_query(int query, enum hp_wmi_command command, void *buffer, > + int insize, int outsize) Can insize and outsize ever be negative? Maybe use u32 or size_t. > +{ > + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL }; > + struct bios_return *bios_return; > + union acpi_object *obj = NULL; > + struct bios_args *args = NULL; > + int mid, actual_outsize; > + size_t bios_args_size; > + int ret; > + > + mid = encode_outsize_for_pvsz(outsize); > + if (WARN_ON(mid < 0)) > + return mid; > + > + bios_args_size = struct_size(args, data, insize); > + args = kmalloc(bios_args_size, GFP_KERNEL); > + if (!args) > + return -ENOMEM; > + > + input.length = bios_args_size; > + input.pointer = args; > + > + args->signature = 0x55434553; What does this number mean? > + args->command = command; > + args->commandtype = query; > + args->datasize = insize; > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > + > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); The driver is mixing calls to the UUID based APIs and the wmi_device ones. wmi_devices is newer and preferred. > + bioscfg_wmi_error_and_message(ret); > + > + if (ret) > + goto out_free; > + > + obj = output.pointer; > + if (!obj) { > + ret = -EINVAL; > + goto out_free; > + } > + if (query != HPWMI_SECUREPLATFORM_GET_STATE && > + command != HPWMI_SECUREPLATFORM) > + if (obj->type != ACPI_TYPE_BUFFER || > + obj->buffer.length < sizeof(*bios_return)) { > + pr_warn("query 0x%x returned wrong type or too small buffer\n", query); > + ret = -EINVAL; > + goto out_free; > + } > + > + > + bios_return = (struct bios_return *)obj->buffer.pointer; For query == HPWMI_SECUREPLATFORM_GET_STATE && command == HPWMI_SECUREPLATFORM this is not guaranteed to be a buffer. > + ret = bios_return->return_code; > + bioscfg_wmi_error_and_message(ret); > + > + if (ret) { > + if (ret != HPWMI_RET_UNKNOWN_COMMAND && > + ret != HPWMI_RET_UNKNOWN_CMDTYPE) > + pr_warn("query 0x%x returned error 0x%x\n", query, ret); > + goto out_free; > + } > + > + /* Ignore output data of zero size */ > + if (!outsize) > + goto out_free; > + > + actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return))); actual_outsize could be negative, which will underflow in the call to memcpy(). > + memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize); > + memset(buffer + actual_outsize, 0, outsize - actual_outsize); memcpy_and_pad() > + > +out_free: > + kfree(obj); > + kfree(args); > + return ret; > +} > + > +static void *utf16_empty_string(u16 *p) > +{ > + *p++ = 2; > + *p++ = (u8)0x00; > + return p; > +} > + > +/* > + * ascii_to_utf16_unicode - Convert ascii string to UTF-16 unicode > + * > + * BIOS supports UTF-16 characters that are 2 bytes long. No variable > + * multi-byte language supported. > + * > + * @p: Unicode buffer address > + * @str: string to convert to unicode > + * > + * Returns a void pointer to the buffer containing unicode string This returns a pointer to the end of the written string. > + */ > +void *ascii_to_utf16_unicode(u16 *p, const u8 *str) > +{ > + int len = strlen(str); > + int ret; > + > + /* > + * Add null character when reading an empty string > + * "02 00 00 00" > + */ > + if (len == 0) > + return utf16_empty_string(p); > + > + /* Move pointer len * 2 number of bytes */ > + *p++ = len * 2; > + ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len); > + if (ret < 0) { > + dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n"); > + goto ascii_to_utf16_unicode_out; > + } What if ret != len ? > + > + if ((ret * sizeof(u16)) > U16_MAX) { > + dev_err(bioscfg_drv.class_dev, "Error string too long\n"); > + goto ascii_to_utf16_unicode_out; > + } > + > +ascii_to_utf16_unicode_out: > + p += len; In cases of errors this will still point to the end of the data that should have been written but was not actually written. The caller has no way to recognize the error case. > + return p; > +} > + > +/* kernel-doc comments start with "/**". Note the two asterisks. > + * hp_wmi_set_bios_setting - Set setting's value in BIOS > + * > + * @input_buffer: Input buffer address > + * @input_size: Input buffer size > + * > + * Returns: Count of unicode characters written to BIOS if successful, otherwise > + * -ENOMEM unable to allocate memory > + * -EINVAL buffer not allocated or too small > + */ > +int hp_wmi_set_bios_setting(u16 *input_buffer, u32 input_size) > +{ > + union acpi_object *obj; > + struct acpi_buffer input = {input_size, input_buffer}; > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > + int ret = 0; No need to initialize "ret". > + > + ret = wmi_evaluate_method(HP_WMI_SET_BIOS_SETTING_GUID, 0, 1, &input, &output); > + > + obj = output.pointer; > + if (!obj) > + return -EINVAL; This skips the bioscfg_wmi_error_and_message call. > + > + if (obj->type != ACPI_TYPE_INTEGER) > + ret = -EINVAL; > + > + ret = obj->integer.value; This overwrites the "ret = -EINVAL" from above. Add an "else" branch. > + bioscfg_wmi_error_and_message(ret); > + > + kfree(obj); > + return ret; > +} > + > +static int bios_attr_set_interface_probe(struct wmi_device *wdev, const void *context) > +{ > + mutex_lock(&bioscfg_drv.mutex); > + bioscfg_drv.bios_attr_wdev = wdev; > + mutex_unlock(&bioscfg_drv.mutex); > + return 0; > +} Technically a WMI UUID can be present multiple times. This would lead to the driver being loaded multiple times, each driver clobbering the bios_attr_wdev of the other drivers. Maybe check the pointer and return -EEXIST. This applies to all subdrivers. > + > +static void bios_attr_set_interface_remove(struct wmi_device *wdev) > +{ > + mutex_lock(&bioscfg_drv.mutex); > + bioscfg_drv.bios_attr_wdev = NULL; > + mutex_unlock(&bioscfg_drv.mutex); > +} > + > +static const struct wmi_device_id bios_attr_set_interface_id_table[] = { > + { .guid_string = HP_WMI_BIOS_GUID}, > + { } > +}; > +static struct wmi_driver bios_attr_set_interface_driver = { > + .driver = { > + .name = DRIVER_NAME > + }, > + .probe = bios_attr_set_interface_probe, > + .remove = bios_attr_set_interface_remove, > + .id_table = bios_attr_set_interface_id_table Put a comma here and above after DRIVER_NAME to reduce future diffs. > +}; > + > +int init_bios_attr_set_interface(void) > +{ > + return wmi_driver_register(&bios_attr_set_interface_driver); > +} > + > +void exit_bios_attr_set_interface(void) > +{ > + wmi_driver_unregister(&bios_attr_set_interface_driver); > +} > + > +MODULE_DEVICE_TABLE(wmi, bios_attr_set_interface_id_table);
Hi Thomas, Please see my comments below. On Sat, Apr 22, 2023 at 4:30 PM Thomas Weißschuh <thomas@t-8ch.de> wrote: > > Hi Jorge, > > checkpatch.pl finds some issues on your patches. > Please make sure checkpath.pl --strict is happy. > I wasn't aware of the '--strict' parameter. It is not part of the help information for checkpath.pl tool. Nonetheless, I will use it forward. Thanks > On 2023-04-20 11:54:42-0500, Jorge Lopez wrote: > > --- > > Based on the latest platform-drivers-x86.git/for-next > > --- > > .../x86/hp/hp-bioscfg/biosattr-interface.c | 307 ++++++++++++++++++ > > 1 file changed, 307 insertions(+) > > create mode 100644 drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c > > > > diff --git a/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c > > new file mode 100644 > > index 000000000000..f09dd41867f7 > > --- /dev/null > > +++ b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c > > @@ -0,0 +1,307 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Functions corresponding to methods under BIOS interface GUID > > + * for use with hp-bioscfg driver. > > + * > > + * Copyright (c) 2022 Hewlett-Packard Inc. > > + */ > > + > > +#include <linux/wmi.h> > > +#include "bioscfg.h" > > + > > +#define SET_DEFAULT_VALUES_METHOD_ID 0x02 > > +#define SET_BIOS_DEFAULTS_METHOD_ID 0x03 > > +#define SET_ATTRIBUTE_METHOD_ID 0x04 > > This could be an enum. Define lines are not in use. They will be removed. > > > + > > +/* > > + * set_attribute() - Update an attribute value > > + * @a_name: The attribute name > > + * @a_value: The attribute value > > + * > > + * Sets an attribute to new value > > + */ > > +int hp_set_attribute(const char *a_name, const char *a_value) > > +{ > > + size_t security_area_size; > > + size_t a_name_size, a_value_size; > > + u16 *buffer = NULL; > > + u16 *start = NULL; > > + int buffer_size; > > + int ret = 0; > > + int instance; > > + char *auth_empty_value = ""; > > + char *auth_token_choice = NULL; > > No need to initialize auth_token_choice and start. > Consider coalescing variable declaration to avoid wasting vertical > space. > Done! > > + > > + > > + mutex_lock(&bioscfg_drv.mutex); > > + if (!bioscfg_drv.bios_attr_wdev) { > > + ret = -ENODEV; > > + goto out_set_attribute; > > + } > > + > > + instance = get_password_instance_for_type(SETUP_PASSWD); > > + if (instance < 0) { > > + ret = -EINVAL; > > + goto out_set_attribute; > > + } > > + > > + if (strlen(bioscfg_drv.password_data[instance].current_password) == 0) > > + strscpy(bioscfg_drv.password_data[instance].current_password, > > + auth_empty_value, > > + sizeof(bioscfg_drv.password_data[instance].current_password)); > > This essentially does > > if (current_password[0] == '\0') > current_password[0] = '\0'; > > ... nothing. > The statement was intended as part of early testing and failed to remove it during cleanup. It will be removed. > > In the driver there is a lot of dereferencing substructures of > bioscfg_drv going on. This makes the code harder to read. > > > + > > + /* Select which auth token to use; password or [auth token] */ > > + > > + if (bioscfg_drv.spm_data.auth_token != NULL) > > + auth_token_choice = bioscfg_drv.spm_data.auth_token; > > + else > > + auth_token_choice = bioscfg_drv.password_data[instance].current_password; > > + > > + a_name_size = bioscfg_calculate_string_buffer(a_name); > > + a_value_size = bioscfg_calculate_string_buffer(a_value); > > + security_area_size = calculate_security_buffer(auth_token_choice); > > + buffer_size = a_name_size + a_value_size + security_area_size; > > + > > + buffer = kmalloc(buffer_size + 1, GFP_KERNEL); > > + if (!buffer) { > > + ret = -ENOMEM; > > + goto out_set_attribute; > > + } > > + > > + /* build variables to set */ > > + start = buffer; > > + start = ascii_to_utf16_unicode(start, a_name); > > + if (!start) > > + goto out_set_attribute; > > ret is 0 here. Is this success? > > > + > > + start = ascii_to_utf16_unicode(start, a_value); > > + if (!start) > > + goto out_set_attribute; > > Same as above. These conditions are not successful. ret value will be reset to indicate the appropriate failure. > > > + > > + populate_security_buffer(start, auth_token_choice); > > + > > + ret = hp_wmi_set_bios_setting(buffer, buffer_size); > > + > > + > > +out_set_attribute: > > + kfree(buffer); > > + mutex_unlock(&bioscfg_drv.mutex); > > + return ret; > > +} > > + > > +/* > > + * hp_wmi_perform_query > > + * > > + * query: The commandtype (enum hp_wmi_commandtype) > > + * write: The command (enum hp_wmi_command) > > + * buffer: Buffer used as input and/or output > > + * insize: Size of input buffer > > + * outsize: Size of output buffer > > + * > > + * returns zero on success > > + * an HP WMI query specific error code (which is positive) > > + * -EINVAL if the query was not successful at all > > + * -EINVAL if the output buffer size exceeds buffersize > > How is the caller supposed to distinguish those? This is a piece of legacy code from early development. 'ret' value is set to -EIO and the line 98 will read " -EIO if the output buffer size exceeds buffersize " > > > + * > > + * Note: The buffersize must at least be the maximum of the input and output > > + * size. E.g. Battery info query is defined to have 1 byte input > > + * and 128 byte output. The caller would do: > > + * buffer = kzalloc(128, GFP_KERNEL); > > + * ret = hp_wmi_perform_query(HPWMI_BATTERY_QUERY, HPWMI_READ, > > + * buffer, 1, 128) > > + */ > > +int hp_wmi_perform_query(int query, enum hp_wmi_command command, void *buffer, > > + int insize, int outsize) > > Can insize and outsize ever be negative? > Maybe use u32 or size_t. The values are positive but there is no check in the event a negative value is passed. I will use u32 instead as precaution. > > > +{ > > + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL }; > > + struct bios_return *bios_return; > > + union acpi_object *obj = NULL; > > + struct bios_args *args = NULL; > > + int mid, actual_outsize; > > + size_t bios_args_size; > > + int ret; > > + > > + mid = encode_outsize_for_pvsz(outsize); > > + if (WARN_ON(mid < 0)) > > + return mid; > > + > > + bios_args_size = struct_size(args, data, insize); > > + args = kmalloc(bios_args_size, GFP_KERNEL); > > + if (!args) > > + return -ENOMEM; > > + > > + input.length = bios_args_size; > > + input.pointer = args; > > + > > + args->signature = 0x55434553; > > What does this number mean? This is a HEX representation of the word 'SECU' expected by BIOS as a signa. > > > + args->command = command; > > + args->commandtype = query; > > + args->datasize = insize; > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > + > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > The driver is mixing calls to the UUID based APIs and the wmi_device > ones. > wmi_devices is newer and preferred. The driver calls wmi_evaluate_method when initiating an WMI call. Where is the driver mixing calls to the UUID based APIs and the wmi_device one? WMI calls are made by calling hp_wmi_perform_query() which invokes wmi_evaluate_method(). Did I miss something? > > > + bioscfg_wmi_error_and_message(ret); > > + > > + if (ret) > > + goto out_free; > > + > > + obj = output.pointer; > > + if (!obj) { > > + ret = -EINVAL; > > + goto out_free; > > + } > > + if (query != HPWMI_SECUREPLATFORM_GET_STATE && > > + command != HPWMI_SECUREPLATFORM) > > + if (obj->type != ACPI_TYPE_BUFFER || > > + obj->buffer.length < sizeof(*bios_return)) { > > + pr_warn("query 0x%x returned wrong type or too small buffer\n", query); > > + ret = -EINVAL; > > + goto out_free; > > + } > > + > > + > > + bios_return = (struct bios_return *)obj->buffer.pointer; > > For query == HPWMI_SECUREPLATFORM_GET_STATE && command == HPWMI_SECUREPLATFORM > this is not guaranteed to be a buffer. BIOS ensures the output is of buffer type and buffer of 1024 bytes in size. This check also help us validate that BIOS only returns a buffer type for this query/command type. > > > + ret = bios_return->return_code; > > + bioscfg_wmi_error_and_message(ret); > > + > > + if (ret) { > > + if (ret != HPWMI_RET_UNKNOWN_COMMAND && > > + ret != HPWMI_RET_UNKNOWN_CMDTYPE) > > + pr_warn("query 0x%x returned error 0x%x\n", query, ret); > > + goto out_free; > > + } > > + > > + /* Ignore output data of zero size */ > > + if (!outsize) > > + goto out_free; > > + > > + actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return))); > > actual_outsize could be negative, which will underflow in the call to > memcpy(). I will evaluate the two size values prior calling memcpy and report an error if needed. > > > + memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize); > > + memset(buffer + actual_outsize, 0, outsize - actual_outsize); > > memcpy_and_pad() > I will replace the two calls with the single proposed memcpy_and_pad call. > > + > > +out_free: > > + kfree(obj); > > + kfree(args); > > + return ret; > > +} > > + > > +static void *utf16_empty_string(u16 *p) > > +{ > > + *p++ = 2; > > + *p++ = (u8)0x00; > > + return p; > > +} > > + > > +/* > > + * ascii_to_utf16_unicode - Convert ascii string to UTF-16 unicode > > + * > > + * BIOS supports UTF-16 characters that are 2 bytes long. No variable > > + * multi-byte language supported. > > + * > > + * @p: Unicode buffer address > > + * @str: string to convert to unicode > > + * > > + * Returns a void pointer to the buffer containing unicode string > > This returns a pointer to the end of the written string. Done > > > + */ > > +void *ascii_to_utf16_unicode(u16 *p, const u8 *str) > > +{ > > + int len = strlen(str); > > + int ret; > > + > > + /* > > + * Add null character when reading an empty string > > + * "02 00 00 00" > > + */ > > + if (len == 0) > > + return utf16_empty_string(p); > > + > > + /* Move pointer len * 2 number of bytes */ > > + *p++ = len * 2; > > + ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len); > > + if (ret < 0) { > > + dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n"); > > + goto ascii_to_utf16_unicode_out; > > + } > > What if ret != len ? only in conditions where utf8s_to_utf16s an error, we can state ret != len. ret == len when utf8s_to_utf16s() is successful. > > > + > > + if ((ret * sizeof(u16)) > U16_MAX) { > > + dev_err(bioscfg_drv.class_dev, "Error string too long\n"); > > + goto ascii_to_utf16_unicode_out; > > + } > > + > > +ascii_to_utf16_unicode_out: > > + p += len; > > In cases of errors this will still point to the end of the data that > should have been written but was not actually written. > The caller has no way to recognize the error case. > That is correct. If an error occurs, we only provide an error message for those conditions. > > + return p; > > +} > > + > > +/* > > kernel-doc comments start with "/**". Note the two asterisks. Done > > > + * hp_wmi_set_bios_setting - Set setting's value in BIOS > > + * > > + * @input_buffer: Input buffer address > > + * @input_size: Input buffer size > > + * > > + * Returns: Count of unicode characters written to BIOS if successful, otherwise > > + * -ENOMEM unable to allocate memory > > + * -EINVAL buffer not allocated or too small > > + */ > > +int hp_wmi_set_bios_setting(u16 *input_buffer, u32 input_size) > > +{ > > + union acpi_object *obj; > > + struct acpi_buffer input = {input_size, input_buffer}; > > + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; > > + int ret = 0; > > No need to initialize "ret". Done! > > > + > > + ret = wmi_evaluate_method(HP_WMI_SET_BIOS_SETTING_GUID, 0, 1, &input, &output); > > + > > + obj = output.pointer; > > + if (!obj) > > + return -EINVAL; > > This skips the bioscfg_wmi_error_and_message call. done! > > > + > > + if (obj->type != ACPI_TYPE_INTEGER) > > + ret = -EINVAL; > > + > > + ret = obj->integer.value; > > This overwrites the "ret = -EINVAL" from above. > Add an "else" branch. done! > > > + bioscfg_wmi_error_and_message(ret); > > + > > + kfree(obj); > > + return ret; > > +} > > + > > +static int bios_attr_set_interface_probe(struct wmi_device *wdev, const void *context) > > +{ > > + mutex_lock(&bioscfg_drv.mutex); > > + bioscfg_drv.bios_attr_wdev = wdev; > > + mutex_unlock(&bioscfg_drv.mutex); > > + return 0; > > +} > > Technically a WMI UUID can be present multiple times. > This would lead to the driver being loaded multiple times, each driver > clobbering the bios_attr_wdev of the other drivers. > > Maybe check the pointer and return -EEXIST. > > This applies to all subdrivers. Done! > > > + > > +static void bios_attr_set_interface_remove(struct wmi_device *wdev) > > +{ > > + mutex_lock(&bioscfg_drv.mutex); > > + bioscfg_drv.bios_attr_wdev = NULL; > > + mutex_unlock(&bioscfg_drv.mutex); > > +} > > + > > +static const struct wmi_device_id bios_attr_set_interface_id_table[] = { > > + { .guid_string = HP_WMI_BIOS_GUID}, > > + { } > > +}; > > +static struct wmi_driver bios_attr_set_interface_driver = { > > + .driver = { > > + .name = DRIVER_NAME > > + }, > > + .probe = bios_attr_set_interface_probe, > > + .remove = bios_attr_set_interface_remove, > > + .id_table = bios_attr_set_interface_id_table > > Put a comma here and above after DRIVER_NAME to reduce future diffs. Done! > > > +}; > > + > > +int init_bios_attr_set_interface(void) > > +{ > > + return wmi_driver_register(&bios_attr_set_interface_driver); > > +} > > + > > +void exit_bios_attr_set_interface(void) > > +{ > > + wmi_driver_unregister(&bios_attr_set_interface_driver); > > +} > > + > > +MODULE_DEVICE_TABLE(wmi, bios_attr_set_interface_id_table);
On 2023-04-24 15:33:26-0500, Jorge Lopez wrote: > Hi Thomas, > > Please see my comments below. > > On Sat, Apr 22, 2023 at 4:30 PM Thomas Weißschuh <thomas@t-8ch.de> wrote: > > > > Hi Jorge, > > > > checkpatch.pl finds some issues on your patches. > > Please make sure checkpath.pl --strict is happy. > > > I wasn't aware of the '--strict' parameter. It is not part of the > help information for checkpath.pl tool. > Nonetheless, I will use it forward. > Thanks It's an alias to --subjective. But indeed, it's hard to see in the help output. > > On 2023-04-20 11:54:42-0500, Jorge Lopez wrote: > > > --- > > > Based on the latest platform-drivers-x86.git/for-next > > No need to initialize auth_token_choice and start. > > Consider coalescing variable declaration to avoid wasting vertical > > space. > > > Done! Please note that this affects many parts of the driver, try to fix it everywhere. > > > +{ > > > + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL }; > > > + struct bios_return *bios_return; > > > + union acpi_object *obj = NULL; > > > + struct bios_args *args = NULL; > > > + int mid, actual_outsize; > > > + size_t bios_args_size; > > > + int ret; > > > + > > > + mid = encode_outsize_for_pvsz(outsize); > > > + if (WARN_ON(mid < 0)) > > > + return mid; > > > + > > > + bios_args_size = struct_size(args, data, insize); > > > + args = kmalloc(bios_args_size, GFP_KERNEL); > > > + if (!args) > > > + return -ENOMEM; > > > + > > > + input.length = bios_args_size; > > > + input.pointer = args; > > > + > > > + args->signature = 0x55434553; > > > > What does this number mean? > This is a HEX representation of the word 'SECU' expected by BIOS as a signa. Sounds like a good thing to comment or put into a #define. > > > > > + args->command = command; > > > + args->commandtype = query; > > > + args->datasize = insize; > > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > > + > > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > > > The driver is mixing calls to the UUID based APIs and the wmi_device > > ones. > > wmi_devices is newer and preferred. > > The driver calls wmi_evaluate_method when initiating an WMI call. > Where is the driver mixing calls to the UUID based APIs and the > wmi_device one? > WMI calls are made by calling hp_wmi_perform_query() which invokes > wmi_evaluate_method(). > Did I miss something? wmi_evaluate_method() is UUID-based. struct wmi_driver is wmi_device based. The wmi_driver/wmi_device code essentially does nothing and is only used to validate that a device is present. The same can be done more easily wmi_has_guid(). > > > > > + bioscfg_wmi_error_and_message(ret); > > > + > > > + if (ret) > > > + goto out_free; > > > + > > > + obj = output.pointer; > > > + if (!obj) { > > > + ret = -EINVAL; > > > + goto out_free; > > > + } > > > + if (query != HPWMI_SECUREPLATFORM_GET_STATE && > > > + command != HPWMI_SECUREPLATFORM) > > > + if (obj->type != ACPI_TYPE_BUFFER || > > > + obj->buffer.length < sizeof(*bios_return)) { > > > + pr_warn("query 0x%x returned wrong type or too small buffer\n", query); > > > + ret = -EINVAL; > > > + goto out_free; > > > + } > > > + > > > + > > > + bios_return = (struct bios_return *)obj->buffer.pointer; > > > > For query == HPWMI_SECUREPLATFORM_GET_STATE && command == HPWMI_SECUREPLATFORM > > this is not guaranteed to be a buffer. > > BIOS ensures the output is of buffer type and buffer of 1024 bytes in > size. This check also help us validate that BIOS only returns a > buffer type for this query/command type. The kernel does not trust the BIOS :-) It trusts nothing and nobody. All cases should be validated. > > > > > + */ > > > +void *ascii_to_utf16_unicode(u16 *p, const u8 *str) > > > +{ > > > + int len = strlen(str); > > > + int ret; > > > + > > > + /* > > > + * Add null character when reading an empty string > > > + * "02 00 00 00" > > > + */ > > > + if (len == 0) > > > + return utf16_empty_string(p); > > > + > > > + /* Move pointer len * 2 number of bytes */ > > > + *p++ = len * 2; > > > + ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len); > > > + if (ret < 0) { > > > + dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n"); > > > + goto ascii_to_utf16_unicode_out; > > > + } > > > > What if ret != len ? > > only in conditions where utf8s_to_utf16s an error, we can state ret != len. > ret == len when utf8s_to_utf16s() is successful. > > > > > + > > > + if ((ret * sizeof(u16)) > U16_MAX) { > > > + dev_err(bioscfg_drv.class_dev, "Error string too long\n"); > > > + goto ascii_to_utf16_unicode_out; > > > + } > > > + > > > +ascii_to_utf16_unicode_out: > > > + p += len; > > > > In cases of errors this will still point to the end of the data that > > should have been written but was not actually written. > > The caller has no way to recognize the error case. > > > That is correct. If an error occurs, we only provide an error message > for those conditions. But the caller has no idea that this error occurred and will try to use the garbage buffer. The error should be communicated to the caller, and the caller has to validate the result. Maybe return NULL? > > > > + return p; > > > +} > > > + > > > +/* > > > > kernel-doc comments start with "/**". Note the two asterisks. > Done This also needs to be done all over the driver.
Hi Thomas, On Mon, Apr 24, 2023 at 4:04 PM Thomas Weißschuh <thomas@t-8ch.de> wrote: > > On 2023-04-24 15:33:26-0500, Jorge Lopez wrote: > > Hi Thomas, > > > > Please see my comments below. > > > > On Sat, Apr 22, 2023 at 4:30 PM Thomas Weißschuh <thomas@t-8ch.de> wrote: > > > > > > Hi Jorge, > > > > > > checkpatch.pl finds some issues on your patches. > > > Please make sure checkpath.pl --strict is happy. > > > > > I wasn't aware of the '--strict' parameter. It is not part of the > > help information for checkpath.pl tool. > > Nonetheless, I will use it forward. > > Thanks > > It's an alias to --subjective. But indeed, it's hard to see in the help > output. Thanks > > > > On 2023-04-20 11:54:42-0500, Jorge Lopez wrote: > > > > --- > > > > Based on the latest platform-drivers-x86.git/for-next > > > No need to initialize auth_token_choice and start. > > > Consider coalescing variable declaration to avoid wasting vertical > > > space. > > > > > Done! > > Please note that this affects many parts of the driver, > try to fix it everywhere. It will be done across all files > > > > > +{ > > > > + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL }; > > > > + struct bios_return *bios_return; > > > > + union acpi_object *obj = NULL; > > > > + struct bios_args *args = NULL; > > > > + int mid, actual_outsize; > > > > + size_t bios_args_size; > > > > + int ret; > > > > + > > > > + mid = encode_outsize_for_pvsz(outsize); > > > > + if (WARN_ON(mid < 0)) > > > > + return mid; > > > > + > > > > + bios_args_size = struct_size(args, data, insize); > > > > + args = kmalloc(bios_args_size, GFP_KERNEL); > > > > + if (!args) > > > > + return -ENOMEM; > > > > + > > > > + input.length = bios_args_size; > > > > + input.pointer = args; > > > > + > > > > + args->signature = 0x55434553; > > > > > > What does this number mean? > > This is a HEX representation of the word 'SECU' expected by BIOS as a signa. > > Sounds like a good thing to comment or put into a #define. I will add a comment since it is only used here. > > > > > > > > + args->command = command; > > > > + args->commandtype = query; > > > > + args->datasize = insize; > > > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > > > + > > > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > > > > > The driver is mixing calls to the UUID based APIs and the wmi_device > > > ones. > > > wmi_devices is newer and preferred. > > > > The driver calls wmi_evaluate_method when initiating an WMI call. > > Where is the driver mixing calls to the UUID based APIs and the > > wmi_device one? > > WMI calls are made by calling hp_wmi_perform_query() which invokes > > wmi_evaluate_method(). > > Did I miss something? > > wmi_evaluate_method() is UUID-based. > struct wmi_driver is wmi_device based. > > The wmi_driver/wmi_device code essentially does nothing and is only used > to validate that a device is present. > The same can be done more easily wmi_has_guid(). > Thank you for the clarification. > > > > > > > + bioscfg_wmi_error_and_message(ret); > > > > + > > > > + if (ret) > > > > + goto out_free; > > > > + > > > > + obj = output.pointer; > > > > + if (!obj) { > > > > + ret = -EINVAL; > > > > + goto out_free; > > > > + } > > > > + if (query != HPWMI_SECUREPLATFORM_GET_STATE && > > > > + command != HPWMI_SECUREPLATFORM) > > > > + if (obj->type != ACPI_TYPE_BUFFER || > > > > + obj->buffer.length < sizeof(*bios_return)) { > > > > + pr_warn("query 0x%x returned wrong type or too small buffer\n", query); > > > > + ret = -EINVAL; > > > > + goto out_free; > > > > + } > > > > + > > > > + > > > > + bios_return = (struct bios_return *)obj->buffer.pointer; > > > > > > For query == HPWMI_SECUREPLATFORM_GET_STATE && command == HPWMI_SECUREPLATFORM > > > this is not guaranteed to be a buffer. > > > > BIOS ensures the output is of buffer type and buffer of 1024 bytes in > > size. This check also help us validate that BIOS only returns a > > buffer type for this query/command type. > > The kernel does not trust the BIOS :-) > It trusts nothing and nobody. > > All cases should be validated. Additional validation will be added to cover all cases. > > > > > > > > + */ > > > > +void *ascii_to_utf16_unicode(u16 *p, const u8 *str) > > > > +{ > > > > + int len = strlen(str); > > > > + int ret; > > > > + > > > > + /* > > > > + * Add null character when reading an empty string > > > > + * "02 00 00 00" > > > > + */ > > > > + if (len == 0) > > > > + return utf16_empty_string(p); > > > > + > > > > + /* Move pointer len * 2 number of bytes */ > > > > + *p++ = len * 2; > > > > + ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len); > > > > + if (ret < 0) { > > > > + dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n"); > > > > + goto ascii_to_utf16_unicode_out; > > > > + } > > > > > > What if ret != len ? > > > > only in conditions where utf8s_to_utf16s an error, we can state ret != len. > > ret == len when utf8s_to_utf16s() is successful. > > > > > > > + > > > > + if ((ret * sizeof(u16)) > U16_MAX) { > > > > + dev_err(bioscfg_drv.class_dev, "Error string too long\n"); > > > > + goto ascii_to_utf16_unicode_out; > > > > + } > > > > + > > > > +ascii_to_utf16_unicode_out: > > > > + p += len; > > > > > > In cases of errors this will still point to the end of the data that > > > should have been written but was not actually written. > > > The caller has no way to recognize the error case. > > > > > That is correct. If an error occurs, we only provide an error message > > for those conditions. > > But the caller has no idea that this error occurred and will try to use > the garbage buffer. > The error should be communicated to the caller, and the caller has to > validate the result. > Maybe return NULL? returning NULL will be a good option so I will review what the impact will be across the driver > > > > > > > + return p; > > > > +} > > > > + > > > > +/* > > > > > > kernel-doc comments start with "/**". Note the two asterisks. > > Done > > This also needs to be done all over the driver. It will be done across all files.
HI Thomas, Sorry for asking again. I just want to be understand exactly what I must do. > > > > > > > > > + args->command = command; > > > > > + args->commandtype = query; > > > > > + args->datasize = insize; > > > > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > > > > + > > > > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > > > > > > > The driver is mixing calls to the UUID based APIs and the wmi_device > > > > ones. > > > > wmi_devices is newer and preferred. > > > > > > The driver calls wmi_evaluate_method when initiating an WMI call. > > > Where is the driver mixing calls to the UUID based APIs and the > > > wmi_device one? > > > WMI calls are made by calling hp_wmi_perform_query() which invokes > > > wmi_evaluate_method(). > > > Did I miss something? > > > > wmi_evaluate_method() is UUID-based. > > struct wmi_driver is wmi_device based. > > > > The wmi_driver/wmi_device code essentially does nothing and is only used > > to validate that a device is present. > > The same can be done more easily wmi_has_guid(). > > > Are you asking to replace all calls to wmi_evaluate_method() which is UUID based API with calls to wmidev_evaluate_method() which is wmi_device based? Correct?
On 2023-04-24 17:14:57-0500, Jorge Lopez wrote: > Sorry for asking again. I just want to be understand exactly what I must do. No problem! > > > > > > > > > > > + args->command = command; > > > > > > + args->commandtype = query; > > > > > > + args->datasize = insize; > > > > > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > > > > > + > > > > > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > > > > > > > > > The driver is mixing calls to the UUID based APIs and the wmi_device > > > > > ones. > > > > > wmi_devices is newer and preferred. > > > > > > > > The driver calls wmi_evaluate_method when initiating an WMI call. > > > > Where is the driver mixing calls to the UUID based APIs and the > > > > wmi_device one? > > > > WMI calls are made by calling hp_wmi_perform_query() which invokes > > > > wmi_evaluate_method(). > > > > Did I miss something? > > > > > > wmi_evaluate_method() is UUID-based. > > > struct wmi_driver is wmi_device based. > > > > > > The wmi_driver/wmi_device code essentially does nothing and is only used > > > to validate that a device is present. > > > The same can be done more easily wmi_has_guid(). > > > > > > > Are you asking to replace all calls to wmi_evaluate_method() which is > UUID based API with calls to wmidev_evaluate_method() which is > wmi_device based? Correct? To be honest I'm not 100% sure. wmi_device is great and perferct for simple drivers binding to a single UUID. But it does not handle multi-UUID logic as your driver needs very well. I would argue to stick to the legacy calls as it allows you to drop a bunch of code and makes the initialization flow more straightforward. But I don't know if somebody else won't ask for wmi_device later.
On Tue, Apr 25, 2023 at 12:28 AM Thomas Weißschuh <thomas@t-8ch.de> wrote: > > On 2023-04-24 17:14:57-0500, Jorge Lopez wrote: > > Sorry for asking again. I just want to be understand exactly what I must do. > > No problem! > > > > > > > > > > > > > > + args->command = command; > > > > > > > + args->commandtype = query; > > > > > > > + args->datasize = insize; > > > > > > > + memcpy(args->data, buffer, flex_array_size(args, data, insize)); > > > > > > > + > > > > > > > + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); > > > > > > > > > > > > The driver is mixing calls to the UUID based APIs and the wmi_device > > > > > > ones. > > > > > > wmi_devices is newer and preferred. > > > > > > > > > > The driver calls wmi_evaluate_method when initiating an WMI call. > > > > > Where is the driver mixing calls to the UUID based APIs and the > > > > > wmi_device one? > > > > > WMI calls are made by calling hp_wmi_perform_query() which invokes > > > > > wmi_evaluate_method(). > > > > > Did I miss something? > > > > > > > > wmi_evaluate_method() is UUID-based. > > > > struct wmi_driver is wmi_device based. > > > > > > > > The wmi_driver/wmi_device code essentially does nothing and is only used > > > > to validate that a device is present. > > > > The same can be done more easily wmi_has_guid(). > > > > > > > > > > > Are you asking to replace all calls to wmi_evaluate_method() which is > > UUID based API with calls to wmidev_evaluate_method() which is > > wmi_device based? Correct? > > To be honest I'm not 100% sure. > > wmi_device is great and perferct for simple drivers binding to a single > UUID. > > But it does not handle multi-UUID logic as your driver needs very well. > > I would argue to stick to the legacy calls as it allows you to drop a > bunch of code and makes the initialization flow more straightforward. > > But I don't know if somebody else won't ask for wmi_device later. I understand. I will keep the legacy code because the driver handles multiple UUID logic. Thank you for the clarification
diff --git a/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c new file mode 100644 index 000000000000..f09dd41867f7 --- /dev/null +++ b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c @@ -0,0 +1,307 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Functions corresponding to methods under BIOS interface GUID + * for use with hp-bioscfg driver. + * + * Copyright (c) 2022 Hewlett-Packard Inc. + */ + +#include <linux/wmi.h> +#include "bioscfg.h" + +#define SET_DEFAULT_VALUES_METHOD_ID 0x02 +#define SET_BIOS_DEFAULTS_METHOD_ID 0x03 +#define SET_ATTRIBUTE_METHOD_ID 0x04 + +/* + * set_attribute() - Update an attribute value + * @a_name: The attribute name + * @a_value: The attribute value + * + * Sets an attribute to new value + */ +int hp_set_attribute(const char *a_name, const char *a_value) +{ + size_t security_area_size; + size_t a_name_size, a_value_size; + u16 *buffer = NULL; + u16 *start = NULL; + int buffer_size; + int ret = 0; + int instance; + char *auth_empty_value = ""; + char *auth_token_choice = NULL; + + + mutex_lock(&bioscfg_drv.mutex); + if (!bioscfg_drv.bios_attr_wdev) { + ret = -ENODEV; + goto out_set_attribute; + } + + instance = get_password_instance_for_type(SETUP_PASSWD); + if (instance < 0) { + ret = -EINVAL; + goto out_set_attribute; + } + + if (strlen(bioscfg_drv.password_data[instance].current_password) == 0) + strscpy(bioscfg_drv.password_data[instance].current_password, + auth_empty_value, + sizeof(bioscfg_drv.password_data[instance].current_password)); + + /* Select which auth token to use; password or [auth token] */ + + if (bioscfg_drv.spm_data.auth_token != NULL) + auth_token_choice = bioscfg_drv.spm_data.auth_token; + else + auth_token_choice = bioscfg_drv.password_data[instance].current_password; + + a_name_size = bioscfg_calculate_string_buffer(a_name); + a_value_size = bioscfg_calculate_string_buffer(a_value); + security_area_size = calculate_security_buffer(auth_token_choice); + buffer_size = a_name_size + a_value_size + security_area_size; + + buffer = kmalloc(buffer_size + 1, GFP_KERNEL); + if (!buffer) { + ret = -ENOMEM; + goto out_set_attribute; + } + + /* build variables to set */ + start = buffer; + start = ascii_to_utf16_unicode(start, a_name); + if (!start) + goto out_set_attribute; + + start = ascii_to_utf16_unicode(start, a_value); + if (!start) + goto out_set_attribute; + + populate_security_buffer(start, auth_token_choice); + + ret = hp_wmi_set_bios_setting(buffer, buffer_size); + + +out_set_attribute: + kfree(buffer); + mutex_unlock(&bioscfg_drv.mutex); + return ret; +} + +/* + * hp_wmi_perform_query + * + * query: The commandtype (enum hp_wmi_commandtype) + * write: The command (enum hp_wmi_command) + * buffer: Buffer used as input and/or output + * insize: Size of input buffer + * outsize: Size of output buffer + * + * returns zero on success + * an HP WMI query specific error code (which is positive) + * -EINVAL if the query was not successful at all + * -EINVAL if the output buffer size exceeds buffersize + * + * Note: The buffersize must at least be the maximum of the input and output + * size. E.g. Battery info query is defined to have 1 byte input + * and 128 byte output. The caller would do: + * buffer = kzalloc(128, GFP_KERNEL); + * ret = hp_wmi_perform_query(HPWMI_BATTERY_QUERY, HPWMI_READ, + * buffer, 1, 128) + */ +int hp_wmi_perform_query(int query, enum hp_wmi_command command, void *buffer, + int insize, int outsize) +{ + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL }; + struct bios_return *bios_return; + union acpi_object *obj = NULL; + struct bios_args *args = NULL; + int mid, actual_outsize; + size_t bios_args_size; + int ret; + + mid = encode_outsize_for_pvsz(outsize); + if (WARN_ON(mid < 0)) + return mid; + + bios_args_size = struct_size(args, data, insize); + args = kmalloc(bios_args_size, GFP_KERNEL); + if (!args) + return -ENOMEM; + + input.length = bios_args_size; + input.pointer = args; + + args->signature = 0x55434553; + args->command = command; + args->commandtype = query; + args->datasize = insize; + memcpy(args->data, buffer, flex_array_size(args, data, insize)); + + ret = wmi_evaluate_method(HP_WMI_BIOS_GUID, 0, mid, &input, &output); + bioscfg_wmi_error_and_message(ret); + + if (ret) + goto out_free; + + obj = output.pointer; + if (!obj) { + ret = -EINVAL; + goto out_free; + } + if (query != HPWMI_SECUREPLATFORM_GET_STATE && + command != HPWMI_SECUREPLATFORM) + if (obj->type != ACPI_TYPE_BUFFER || + obj->buffer.length < sizeof(*bios_return)) { + pr_warn("query 0x%x returned wrong type or too small buffer\n", query); + ret = -EINVAL; + goto out_free; + } + + + bios_return = (struct bios_return *)obj->buffer.pointer; + ret = bios_return->return_code; + bioscfg_wmi_error_and_message(ret); + + if (ret) { + if (ret != HPWMI_RET_UNKNOWN_COMMAND && + ret != HPWMI_RET_UNKNOWN_CMDTYPE) + pr_warn("query 0x%x returned error 0x%x\n", query, ret); + goto out_free; + } + + /* Ignore output data of zero size */ + if (!outsize) + goto out_free; + + actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return))); + memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize); + memset(buffer + actual_outsize, 0, outsize - actual_outsize); + +out_free: + kfree(obj); + kfree(args); + return ret; +} + +static void *utf16_empty_string(u16 *p) +{ + *p++ = 2; + *p++ = (u8)0x00; + return p; +} + +/* + * ascii_to_utf16_unicode - Convert ascii string to UTF-16 unicode + * + * BIOS supports UTF-16 characters that are 2 bytes long. No variable + * multi-byte language supported. + * + * @p: Unicode buffer address + * @str: string to convert to unicode + * + * Returns a void pointer to the buffer containing unicode string + */ +void *ascii_to_utf16_unicode(u16 *p, const u8 *str) +{ + int len = strlen(str); + int ret; + + /* + * Add null character when reading an empty string + * "02 00 00 00" + */ + if (len == 0) + return utf16_empty_string(p); + + /* Move pointer len * 2 number of bytes */ + *p++ = len * 2; + ret = utf8s_to_utf16s(str, strlen(str), UTF16_HOST_ENDIAN, p, len); + if (ret < 0) { + dev_err(bioscfg_drv.class_dev, "UTF16 conversion failed\n"); + goto ascii_to_utf16_unicode_out; + } + + if ((ret * sizeof(u16)) > U16_MAX) { + dev_err(bioscfg_drv.class_dev, "Error string too long\n"); + goto ascii_to_utf16_unicode_out; + } + +ascii_to_utf16_unicode_out: + p += len; + return p; +} + +/* + * hp_wmi_set_bios_setting - Set setting's value in BIOS + * + * @input_buffer: Input buffer address + * @input_size: Input buffer size + * + * Returns: Count of unicode characters written to BIOS if successful, otherwise + * -ENOMEM unable to allocate memory + * -EINVAL buffer not allocated or too small + */ +int hp_wmi_set_bios_setting(u16 *input_buffer, u32 input_size) +{ + union acpi_object *obj; + struct acpi_buffer input = {input_size, input_buffer}; + struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL}; + int ret = 0; + + ret = wmi_evaluate_method(HP_WMI_SET_BIOS_SETTING_GUID, 0, 1, &input, &output); + + obj = output.pointer; + if (!obj) + return -EINVAL; + + if (obj->type != ACPI_TYPE_INTEGER) + ret = -EINVAL; + + ret = obj->integer.value; + bioscfg_wmi_error_and_message(ret); + + kfree(obj); + return ret; +} + +static int bios_attr_set_interface_probe(struct wmi_device *wdev, const void *context) +{ + mutex_lock(&bioscfg_drv.mutex); + bioscfg_drv.bios_attr_wdev = wdev; + mutex_unlock(&bioscfg_drv.mutex); + return 0; +} + +static void bios_attr_set_interface_remove(struct wmi_device *wdev) +{ + mutex_lock(&bioscfg_drv.mutex); + bioscfg_drv.bios_attr_wdev = NULL; + mutex_unlock(&bioscfg_drv.mutex); +} + +static const struct wmi_device_id bios_attr_set_interface_id_table[] = { + { .guid_string = HP_WMI_BIOS_GUID}, + { } +}; +static struct wmi_driver bios_attr_set_interface_driver = { + .driver = { + .name = DRIVER_NAME + }, + .probe = bios_attr_set_interface_probe, + .remove = bios_attr_set_interface_remove, + .id_table = bios_attr_set_interface_id_table +}; + +int init_bios_attr_set_interface(void) +{ + return wmi_driver_register(&bios_attr_set_interface_driver); +} + +void exit_bios_attr_set_interface(void) +{ + wmi_driver_unregister(&bios_attr_set_interface_driver); +} + +MODULE_DEVICE_TABLE(wmi, bios_attr_set_interface_id_table);
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/biosattr-interface.c | 307 ++++++++++++++++++ 1 file changed, 307 insertions(+) create mode 100644 drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c