Hi,
On 2/18/22 17:09, 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 ZBook Workstation notebook. Additional
> validation was included to ensure no other commands were failing or
> 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 | 67 +++++++++++++++++++++++------------
> 1 file changed, 44 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c
> index de715687021a..11c8e9b6e64a 100644
> --- a/drivers/platform/x86/hp-wmi.c
> +++ b/drivers/platform/x86/hp-wmi.c
> @@ -83,12 +83,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[0];
> };
>
> enum hp_wmi_commandtype {
> @@ -258,39 +263,47 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
> 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 };
> + size_t bios_args_size = sizeof(struct bios_args) + insize;
> + struct bios_args *args = NULL;
> + 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;
>
> - 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 in_free;
> + }
>
> - obj = output.pointer;
> + /* Avoid unnecessary copy to the data buffer if input buffer size is zero */
> + if (insize > 0)
This if is not necessary, memcpy should touch neither dst nor src when
called with size==0.
> + memcpy(args->data, buffer, 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);
Add:
if (ret)
goto in_free;
Here ? AFAIK if ret is set &output is not touched, so other
wise you end up reading unitialized mem when doing:
obj = output.pointer;
> + if (!obj) {
> + pr_warn("query 0x%x returned a null obj 0x%x\n", query, ret);
Are you sure this can never happen when e.g. querying features on
older models ? I believe it is probably best to just drop this as
it is an unrelated behavior change and if you really think this is
a good idea, put the adding of the pr_warn in a separate commit
with an explanation why this is being added.
> ret = -EINVAL;
> - goto out_free;
> + goto in_free;
> }
>
> - bios_return = (struct bios_return *)obj->buffer.pointer;
> - ret = bios_return->return_code;
> + if (!ret && obj->type == ACPI_TYPE_BUFFER) {
> + bios_return = (struct bios_return *)obj->buffer.pointer;
> + ret = bios_return->return_code;
> + }
Please keep this in the form of:
if (unexpected_condition) {
ret = -EINVAL;
goto out_free;
}
straight-path-statements-here;
This form is consistently used in most kernel code so that the
"straight-path" code is always indented only 1 level which makes
it easier to follow.
And this would also mean that you end up making less changes
in this patch.
>
> if (ret) {
> if (ret != HPWMI_RET_UNKNOWN_COMMAND &&
> @@ -299,6 +312,12 @@ 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;
> + }
> +
> /* Ignore output data of zero size */
> if (!outsize)
> goto out_free;
> @@ -309,6 +328,8 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command,
>
> out_free:
> kfree(obj);
> +in_free:
> + kfree(args);
> return ret;
IMHO it would be better to have a single "out" label here and
initialize obj to NULL when declared, kfree(NULL) is a no-op,
so this way you don't need 2 labels and you also avoid potentially
jumping to the wrong label.
Regards,
Hans
