Re: [PATCH v4 2/6] Introduction of HP-BIOSCFG driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

> Regards,
>
> Hans
>
>




[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux