| Message ID | 20220218160907.3422-4-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix SW_TABLET_MODE detection method | expand |
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@hp.com> > > --- > 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
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) + 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); + + obj = output.pointer; + if (!obj) { + pr_warn("query 0x%x returned a null obj 0x%x\n", query, ret); 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; + } 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; }
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@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- drivers/platform/x86/hp-wmi.c | 67 +++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 23 deletions(-)