Hi,
On 3/14/22 11:51, Hans de Goede wrote:
> Hi,
>
> On 3/10/22 22:08, Jorge Lopez wrote:
>> The purpose of this patch is to remove 128 bytes buffer limitation
>> imposed in bios_args structure.
>>
>> A limiting factor discovered during this investigation was the struct
>> bios_args.data size restriction. The data member size limits all
>> possible WMI commands to those requiring buffer size of 128 bytes or
>> less. Several WMI commands and queries require a buffer size larger
>> than 128 bytes hence limiting current and feature supported by the
>> driver. It is for this reason, struct bios_args.data changed and is
>> dynamically allocated. hp_wmi_perform_query function changed to
>> handle the memory allocation and release of any required buffer size.
>>
>> All changes were validated on a HP ZBook Workstation notebook,
>> HP EliteBook x360, and HP EliteBook 850 G8. Additional
>> validation was included in the test process to ensure no other
>> commands were incorrectly handled.
>>
>> Signed-off-by: Jorge Lopez <jorge.lopez2@xxxxxx>
>>
>> ---
>> Based on the latest platform-drivers-x86.git/for-next
>> ---
>> drivers/platform/x86/hp-wmi.c | 64 +++++++++++++++++++++++------------
>> 1 file changed, 42 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
>> index e76bd4bef6b5..cc5c4f637328 100644
>> --- a/drivers/platform/x86/hp-wmi.c
>> +++ b/drivers/platform/x86/hp-wmi.c
>> @@ -82,12 +82,17 @@ enum hp_wmi_event_ids {
>> HPWMI_BATTERY_CHARGE_PERIOD = 0x10,
>> };
>>
>> +/**
>> + * struct bios_args buffer is dynamically allocated. New WMI command types
>> + * were introduced that exceeds 128-byte data size. Changes to handle
>> + * the data size allocation scheme were kept in hp_wmi_perform_qurey function.
>> + */
>> struct bios_args {
>> u32 signature;
>> u32 command;
>> u32 commandtype;
>> u32 datasize;
>> - u8 data[128];
>> + u8 data[];
>> };
>>
>> enum hp_wmi_commandtype {
>> @@ -268,34 +273,40 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>> int mid;
>> struct bios_return *bios_return;
>> int actual_outsize;
>> - union acpi_object *obj;
>> - struct bios_args args = {
>> - .signature = 0x55434553,
>> - .command = command,
>> - .commandtype = query,
>> - .datasize = insize,
>> - .data = { 0 },
>> - };
>> - struct acpi_buffer input = { sizeof(struct bios_args), &args };
>> + union acpi_object *obj = NULL;
>> + struct bios_args *args = NULL;
>> + size_t bios_args_size = struct_size(args, data, insize);
>> +
>> + struct acpi_buffer input;
>> struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
>> int ret = 0;
>>
>> - mid = encode_outsize_for_pvsz(outsize);
>> - if (WARN_ON(mid < 0))
>> - return mid;
>> + args = kmalloc(bios_args_size, GFP_KERNEL);
>> + if (!args)
>> + return -ENOMEM;
>
> The variable declaration here again looks a bit messy, also
> there is no reason to move the block setting + checking mid,
> that just makes the diff unnecessarily large.
>
> I've cleaned this up while mergin (no functional changes).
>
>
>>
>> - if (WARN_ON(insize > sizeof(args.data)))
>> - return -EINVAL;
>> - memcpy(&args.data[0], buffer, insize);
>> + input.length = bios_args_size;
>> + input.pointer = args;
>>
>> - wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
>> + mid = encode_outsize_for_pvsz(outsize);
>> + if (WARN_ON(mid < 0)) {
>> + ret = mid;
>> + goto out_free;
>> + }
>>
>> - obj = output.pointer;
>> + memcpy(args->data, buffer, flex_array_size(args, data, insize));
>>
>> - if (!obj)
>> - return -EINVAL;
>> + args->signature = 0x55434553;
>> + args->command = command;
>> + args->commandtype = query;
>> + args->datasize = insize;
>>
>> - if (obj->type != ACPI_TYPE_BUFFER) {
>> + ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
>> + if (ret)
>> + goto out_free;
>> +
>> + obj = output.pointer;
>> + if (!obj) {
>> ret = -EINVAL;
>> goto out_free;
>> }
>> @@ -310,9 +321,17 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>> goto out_free;
>> }
>>
>> + if (obj->type != ACPI_TYPE_BUFFER) {
>> + pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
>> + ret = -EINVAL;
>> + goto out_free;
>> + }
>> +
>
> You have now moved the obj->type != ACPI_TYPE_BUFFER check to after the:
>
> bios_return = (struct bios_return *)obj->buffer.pointer;
>
> Statement, which dereferences the buffer member of the obj union. That check
> MUST be done before looking at the buffer member, so I have moved it back to
> its old place, this also makes the diff/patch smaller. Note this one is
> a functional change to your patch.
>
> The changed initial block of the function now looks like this:
>
> static 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 };
> size_t bios_args_size = struct_size(args, data, insize);
Ahum, there is a compile error here because args is not declared yet, so the final
version looks slightly different, see:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/commit/?h=review-hans&id=9f7e383ebdee6712bee02e3a6c2027cf287950fc
Regards,
Hans
> struct bios_return *bios_return;
> union acpi_object *obj = NULL;
> struct bios_args *args = NULL;
> int mid, actual_outsize, ret;
>
> mid = encode_outsize_for_pvsz(outsize);
> if (WARN_ON(mid < 0))
> return mid;
>
> 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(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> if (ret)
> goto out_free;
>
> obj = output.pointer;
> if (!obj) {
> ret = -EINVAL;
> goto out_free;
> }
>
> if (obj->type != ACPI_TYPE_BUFFER) {
> pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
> ret = -EINVAL;
> goto out_free;
> }
>
> bios_return = (struct bios_return *)obj->buffer.pointer;
>
>
> And the new diff for this chunk of the patch now is:
>
> @@ -266,37 +271,42 @@ static inline int encode_outsize_for_pvsz(int outsize)
> static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
> void *buffer, int insize, int outsize)
> {
> - int mid;
> + struct acpi_buffer input, output = { ACPI_ALLOCATE_BUFFER, NULL };
> + size_t bios_args_size = struct_size(args, data, insize);
> struct bios_return *bios_return;
> - int actual_outsize;
> - union acpi_object *obj;
> - struct bios_args args = {
> - .signature = 0x55434553,
> - .command = command,
> - .commandtype = query,
> - .datasize = insize,
> - .data = { 0 },
> - };
> - struct acpi_buffer input = { sizeof(struct bios_args), &args };
> - struct acpi_buffer output = { ACPI_ALLOCATE_BUFFER, NULL };
> - int ret = 0;
> + union acpi_object *obj = NULL;
> + struct bios_args *args = NULL;
> + int mid, actual_outsize, ret;
>
> mid = encode_outsize_for_pvsz(outsize);
> if (WARN_ON(mid < 0))
> return mid;
>
> - if (WARN_ON(insize > sizeof(args.data)))
> - return -EINVAL;
> - memcpy(&args.data[0], buffer, insize);
> + args = kmalloc(bios_args_size, GFP_KERNEL);
> + if (!args)
> + return -ENOMEM;
>
> - wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> + input.length = bios_args_size;
> + input.pointer = args;
>
> - obj = output.pointer;
> + args->signature = 0x55434553;
> + args->command = command;
> + args->commandtype = query;
> + args->datasize = insize;
> + memcpy(args->data, buffer, flex_array_size(args, data, insize));
>
> - if (!obj)
> - return -EINVAL;
> + ret = wmi_evaluate_method(HPWMI_BIOS_GUID, 0, mid, &input, &output);
> + if (ret)
> + goto out_free;
> +
> + obj = output.pointer;
> + if (!obj) {
> + ret = -EINVAL;
> + goto out_free;
> + }
>
> if (obj->type != ACPI_TYPE_BUFFER) {
> + pr_warn("query 0x%x returned an invalid object 0x%x\n", query, ret);
> ret = -EINVAL;
> goto out_free;
> }
> @@ ...
>
> I've merged this patch with the above changes:
>
> Thank you for your patch, I've applied this patch to my review-hans
> branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
>
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.
>
> Regards,
>
> Hans
>
>
>
>
>
>
>
>
>> /* Ignore output data of zero size */
>> - if (!outsize)
>> + if (!outsize) {
>> + ret = 0;
>> goto out_free;
>> + }
>>
>> actual_outsize = min(outsize, (int)(obj->buffer.length - sizeof(*bios_return)));
>> memcpy(buffer, obj->buffer.pointer + sizeof(*bios_return), actual_outsize);
>> @@ -320,6 +339,7 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>>
>> out_free:
>> kfree(obj);
>> + kfree(args);
>> return ret;
>> }
>>
>
>
