| Message ID | 20220310210853.28367-5-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | Fix SW_TABLET_MODE detection method | expand |
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@hp.com> > > --- > 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); 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; > } >
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@hp.com> >> >> --- >> 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; >> } >> > >
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; - 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; + } + /* 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; }
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@hp.com> --- Based on the latest platform-drivers-x86.git/for-next --- drivers/platform/x86/hp-wmi.c | 64 +++++++++++++++++++++++------------ 1 file changed, 42 insertions(+), 22 deletions(-)