Hi Hans,
On Wed, Nov 9, 2022 at 12:10 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>
> Hi,
>
> On 11/9/22 18:24, Jorge Lopez wrote:
> > HI Hans,
> >
> > Please see questions and comments below.
> >
> > On Tue, Nov 8, 2022 at 8:51 AM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> >>
> >> Hi Jorge,
> >>
> >> Review comments inline.
> >>
> >> On 10/20/22 22:10, Jorge Lopez wrote:
> >>> The purpose for this patch is submit HP BIOSCFG driver to be list of
> >>> HP Linux kernel drivers. The driver include a total of 12 files
> >>> broken in several patches. This is set 1 of 4.
> >>>
> >>> 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 PC’s 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 BISOCFG 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 features such Secure Platform Management, Sure
> >>> Admin, and Sure Start. Each section provides security feature
> >>> description and identifies sysfs directories and files exposed by
> >>> the driver.
> >>>
> >>> Many HP Commercial PC’s 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 PC’s 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 Sure Admin cryptographic signature-based authorization string
> >>> that the BIOS will use to verify authorization to modify the setting.
> >>>
> >>> Signed-off-by: Jorge Lopez <jorge.lopez2@xxxxxx>
> >>>
> >>> ---
> >>> Based on the latest platform-drivers-x86.git/for-next
> >>> ---
> >>> .../x86/hp/hp-bioscfg/biosattr-interface.c | 285 ++++++++
> >>> drivers/platform/x86/hp/hp-bioscfg/bioscfg.h | 671 ++++++++++++++++++
> >>> .../x86/hp/hp-bioscfg/enum-attributes.c | 521 ++++++++++++++
> >>> 3 files changed, 1477 insertions(+)
> >>> create mode 100644 drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c
> >>> create mode 100644 drivers/platform/x86/hp/hp-bioscfg/bioscfg.h
> >>> create mode 100644 drivers/platform/x86/hp/hp-bioscfg/enum-attributes.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..f0c919bf3ab0
> >>> --- /dev/null
> >>> +++ b/drivers/platform/x86/hp/hp-bioscfg/biosattr-interface.c
> >>> @@ -0,0 +1,285 @@
> >>> +// 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;
> >>
> >> You have 2 spaces between int and buffer_size here, please drop
> >> one.
> >>
> >>
> >>> + int ret;
> >>> + int instance;
> >>> + char *auth_empty_value = " ";
> >>> +
> >>> + 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)
> >>> + goto out_set_attribute;
> >>> +
> >>> + if (strlen(bioscfg_drv.password_data[instance].current_password) == 0)
> >>> + strncpy(bioscfg_drv.password_data[instance].current_password,
> >>> + auth_empty_value,
> >>> + sizeof(bioscfg_drv.password_data[instance].current_password));
> >>
> >> strncpy does not guarantee 0 termination of the destination buffer,
> >> please use strscpy.
> >>
> >>> +
> >>> + a_name_size = calculate_string_buffer(a_name);
> >>> + a_value_size = calculate_string_buffer(a_value);
> >>> + security_area_size = calculate_security_buffer(bioscfg_drv.password_data[instance].current_password);
> >>> + buffer_size = a_name_size + a_value_size + security_area_size;
> >>> +
> >>> + buffer = kzalloc(buffer_size, 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, bioscfg_drv.password_data[instance].current_password);
> >>> + 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_insize, actual_outsize;
> >>> + size_t bios_args_size;
> >>> + int ret;
> >>> +
> >>> + mid = encode_outsize_for_pvsz(outsize);
> >>> + if (WARN_ON(mid < 0))
> >>> + return mid;
> >>> +
> >>> + actual_insize = insize;
> >>> + 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_drv.last_wmi_status = ret;
> >>> + if (ret)
> >>> + goto out_free;
> >>> +
> >>> + obj = output.pointer;
> >>> + if (!obj) {
> >>> + ret = -EINVAL;
> >>> + goto out_free;
> >>> + }
> >>> +
> >>
> >> You need to check the type of obj here before dereferencing
> >> obj as if it is a buffer.
> >>
> >>> + bios_return = (struct bios_return *)obj->buffer.pointer;
> >>> + ret = bios_return->return_code;
> >>> + bioscfg_drv.last_wmi_status = 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;
> >>> +}
> >>> +
> >>> +/*
> >>> + * ascii_to_utf16_unicode - Convert ascii string to UTF-16 unicode
> >>> + *
> >>> + * @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
> >>> + */
> >>> + if (len == 0) {
> >>> + *p++ = 2;
> >>> + *p++ = (u8)0x00;
> >>> + return p;
> >>
> >> This does not match with calculate_string_buffer() which will
> >> return 2 for a 0 length string while you are using 4 bytes here.
> >>
> >> I guess this may also be why you need to use " " for
> >> auth_empty_value above, so as to avoid this bug.
> >>
> > HP BIOS expects 2 characters when an empty string is being converted
> > to u16 hence the reason for returning 2 instead of zero. This is an
> > intended behavior and needed when allocating a buffer and writing to
> > BIOS.
>
> Right I understand that, it wants 2 characters for the 16 bit length
> word, but why not write 0 to that 16 bit length word. Why actually
> say the string-buffer length is 2 bytes long / and then write
> a 16-bit word with value 0?
>
> What you are doing now creates a 4 byte buffer like this:
>
> u8 buf[4] = { 0x02, 0x00, 0x00, 0x00 }
>
> Why not just create a 2 byte buffer like this:
>
> u8 buf[2] = { 0x00, 0x00 }
>
> ?
>
>
> Also I'm wondering why the empty auth string is " " and
> not "" ?
>
>
The string returned for an empty string is 4 bytes. The returned
string includes two bytes for the string size in bytes and the
remaining bytes are the string.
Size = 0x02, 0x00
String = 0x00, 0x00
All strings return include the string size in bytes followed by the u16 string
> >
> >>
> >>> + }
> >>> + *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;
> >>
> >> You have an error here, but you don't return an error at the end of
> >> this function.
> >>
> >> Please for version 5 do the following:
> >>
> >> 1. Add a preparation patch which moves populate_string_buffer()
> >> and calculate_string_buffer() from
> >> drivers/platform/x86/dell/dell-wmi-sysman/sysman.c to
> >> drivers/platform/x86/wmi.c
> >>
> > Are you asking to change sysman.c which is a DELL specific driver?
> > I don't have a DELL platform to validate the changes and I will be
> > doing the work on HP workday. Sorry but I cannot do that.
>
> I'm asking you to move the functions to drivers/platform/x86/wmi.c,
> rename them and check they still compile. So that these functions
> can be shared. You won't be changing the code at all, just the function
> names.
>
> And I have a Dell Latitude laptop where I can verify that
> dell-wmi-sysman still works.
>
>
>
> >
> >> Renaming them to:
> >>
> >> size_t wmi_utf16_str_size(const char *str);
> >> ssize_t wmi_str_to_utf16_str(u16 *buffer, size_t buffer_len, const char *str);
> >>
> >> (adding these prototypes to include/linux/wmi.h)
> >>
> >
> > I will make the changes requested but I'll wait for your response to
> > the previous comments regarding calculate_string_buffer()
>
> Ok.
>
>
> Regards,
>
> Hans
>
>
> p.s.
>
> Next time if you are replying to a really long email like my previous
> one, please remove any not relevant quoted text from the reply, to
> make it easier to find the parts where you actually reply.
>
ok.
>
