| Message ID | 20220307220932.23607-4-jorge.lopez2@hp.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Fix SW_TABLET_MODE detection method | expand |
[Public] > -----Original Message----- > From: Jorge Lopez <jorgealtxwork@gmail.com> > Sent: Monday, March 7, 2022 16:10 > To: platform-driver-x86@vger.kernel.org > Subject: [PATCH v4 3/3] Changing bios_args.data to be dynamically allocated > > 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 Was this meant as "changes from v3->v4"? Or it's just a comment? > --- No need for double --. Just put everything commentary below the one --. > drivers/platform/x86/hp-wmi.c | 59 ++++++++++++++++++++++------------- > 1 file changed, 38 insertions(+), 21 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index a0aba7db8a1c..a04723fdea60 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[0]; > }; > > enum hp_wmi_commandtype { > @@ -268,34 +273,39 @@ 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; > + 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 out_free; > + } > > - obj = output.pointer; > + 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); > + if (ret) > + goto out_free; > + > + obj = output.pointer; > + if (!obj) { > ret = -EINVAL; > goto out_free; > } > @@ -310,6 +320,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; > @@ -320,6 +336,7 @@ static int hp_wmi_perform_query(int query, enum > hp_wmi_command command, > > out_free: > kfree(obj); > + kfree(args); > return ret; > } > > -- > 2.25.1
[Public]
> The only changes apply to V3-0001. Patches V3-0002 and V3-0003 did not changed in version V4.
In general it's nice to include either in your cover letter "changes from X-Y" or in individual patches below the --.
It makes it easier for anyone who reviewed previous version to spot the differences.
Hi, On 3/7/22 23:12, Limonciello, Mario wrote: > [Public] > > > >> -----Original Message----- >> From: Jorge Lopez <jorgealtxwork@gmail.com> >> Sent: Monday, March 7, 2022 16:10 >> To: platform-driver-x86@vger.kernel.org >> Subject: [PATCH v4 3/3] Changing bios_args.data to be dynamically allocated >> >> 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 > > Was this meant as "changes from v3->v4"? Or it's just a comment? > >> --- > > No need for double --. Just put everything commentary below the one --. The second cut-line gets added by git format-patch, there is nothing which can be done about that. Regards, Hans > >> drivers/platform/x86/hp-wmi.c | 59 ++++++++++++++++++++++------------- >> 1 file changed, 38 insertions(+), 21 deletions(-) >> >> diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c >> index a0aba7db8a1c..a04723fdea60 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[0]; >> }; >> >> enum hp_wmi_commandtype { >> @@ -268,34 +273,39 @@ 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; >> + 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 out_free; >> + } >> >> - obj = output.pointer; >> + 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); >> + if (ret) >> + goto out_free; >> + >> + obj = output.pointer; >> + if (!obj) { >> ret = -EINVAL; >> goto out_free; >> } >> @@ -310,6 +320,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; >> @@ -320,6 +336,7 @@ static int hp_wmi_perform_query(int query, enum >> hp_wmi_command command, >> >> out_free: >> kfree(obj); >> + kfree(args); >> return ret; >> } >> >> -- >> 2.25.1 >
Hi, On 3/7/22 23: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 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 | 59 ++++++++++++++++++++++------------- > 1 file changed, 38 insertions(+), 21 deletions(-) > > diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c > index a0aba7db8a1c..a04723fdea60 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[0]; This should be: u8 data[]; See: https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays > }; > > enum hp_wmi_commandtype { > @@ -268,34 +273,39 @@ 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; > + size_t bios_args_size = sizeof(struct bios_args) + insize; Please use the struct_size() macro for this, see the end of: https://docs.kernel.org/process/deprecated.html#zero-length-and-one-element-arrays for an example. > + 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 out_free; > + } > > - obj = output.pointer; > + 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); > + if (ret) > + goto out_free; > + > + obj = output.pointer; > + if (!obj) { > ret = -EINVAL; > goto out_free; > } > @@ -310,6 +320,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; Should we maybe have a "ret = -EINVAL" here, or maybe ret = 0 ? Or is ret always 0 here already ? > @@ -320,6 +336,7 @@ static int hp_wmi_perform_query(int query, enum hp_wmi_command command, > > out_free: > kfree(obj); > + kfree(args); > return ret; > } > Otherwise this looks good to me. Regards, Hans
diff --git a/drivers/platform/x86/hp-wmi.c b/drivers/platform/x86/hp-wmi.c index a0aba7db8a1c..a04723fdea60 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[0]; }; enum hp_wmi_commandtype { @@ -268,34 +273,39 @@ 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; + 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 out_free; + } - obj = output.pointer; + 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); + if (ret) + goto out_free; + + obj = output.pointer; + if (!obj) { ret = -EINVAL; goto out_free; } @@ -310,6 +320,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; @@ -320,6 +336,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 | 59 ++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 21 deletions(-)