Hi,
On 11/9/22 22:04, Jorge Lopez wrote:
> On Wed, Nov 9, 2022 at 2:55 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 11/9/22 21:52, Jorge Lopez wrote:
>>> On Wed, Nov 9, 2022 at 2:05 PM Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 11/9/22 21:00, Jorge Lopez wrote:
>>>>> 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
>>>>
>>>> Right I understand that, but why is the "String = 0x00, 0x00"
>>>> there ? All the non-0-length strings are not 0 terminated,
>>>> why does the zero length string needs to be specified as length 2
>>>> (1 u16) and then have that u16 be a 0 terminator ?
>>>>
>>> It is a specific format required by BIOS for any zero-length strings.
>>> I don't know the reason why BIOS wants that format.
>>>
>>>
>>>> Have you tried just using Size[2] = 0x00, 0x00 and String[0] for
>>>> an empty string?
>>>>
>>>
>>> I tried during the development process and each time the data is
>>> rejected by BIOS.
>>
>> Ok, well in that case you are going to need your own
>> ascii_to_utf16_unicode() to handle the weird case for the
>> 0 sized string, so no need to move the dell-wmi-sysman versions
>> to the shared wmi code.
>>
>> But please do start with a copy of the Dell function and then
>> add the special case for the 0 len string, since the original
>> version above does not properly handle errors.
>>
>> Also this means you need to update the hp calculate_string_buffer()
>> copy to properly return 4 rather then 2 for the bufsize for
>> a 0 length string.
>>
>
> I will do so. Thanks.
> BTW, changes requested for sysman.c will require changes to two other
> DELL source files;
> biosattr-interface.c and passwordattr-interface.c.
> All I can do is to verify it compiles ok.
Note that since you now need a special version to deal
with the 0 length string weirdness, you can just make
a private copy of the dell functions into the hp code
and then modify that to add the special case for the
0 length strings.
So there is no longer a need to move the dell versions
out into wmi.c since the hp code will need a modified
version anyways (so the code cannot be shared).
Regards,
Hans
