Hi Jorge,
On 8/1/23 21:16, Jorge Lopez wrote:
> Replace method how the string length is calculated.
> Removed unused variable 'size'
>
> Signed-off-by: Jorge Lopez <jorge.lopez2@xxxxxx>
While reviewing this I have noticed that the parsing of ORD_LIST_ELEMENTS
in hp_populate_ordered_list_elements_from_package() seems to be quite buggy:
1. Normally str_value and value_len get set for string type package elements by:
case ACPI_TYPE_STRING:
if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer,
order_obj[elem].string.length,
&str_value, &value_len);
if (ret)
continue;
}
break;
But notice how the hp_convert_hexstr_to_str() call gets stepped when
elem == ORD_LIST_ELEMENTS .
Yes when next dealing with ORD_LIST_ELEMENTS the never updated str_value and value_len
get used:
switch (eloc) {
...
case ORD_LIST_ELEMENTS:
/*
* Ordered list data is stored in hex and comma separated format
* Convert the data and split it to show each element
*/
ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len);
if (ret)
goto exit_list;
So that does not seem right.
2. ordered_list_data->elements[0] never gets filled when there actually is a comma in
the ordered-list, iow when there is more then 1 element:
part_tmp = tmpstr;
part = strsep(&part_tmp, COMMA_SEP);
if (!part)
strscpy(ordered_list_data->elements[0],
tmpstr,
sizeof(ordered_list_data->elements[0]));
for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) {
strscpy(ordered_list_data->elements[elem],
part,
sizeof(ordered_list_data->elements[elem]));
part = strsep(&part_tmp, SEMICOLON_SEP);
}
Notice how the for starts at elem = 1, so if part is not NULL (and it is never NULL for the first call strsep will always return tmpstr) then ordered_list_data->elements[0] never gets filled.
3. ordered_list_data->elements_size is set but never validated. You should compare elem after the loop with ordered_list_data->elements_size and make sure they match. IOW verify that 0-(ordered_list_data->elements_size-1) entries of the ordered_list_data->elements[] array have been filled.
4. For specific values of eloc the code expects the current order_obj[elem] to be either an integer or a string, but this is not validated. Please validate that order_obj[elem].type matches with what is expected (string or int) for the current value of eloc.
This all makes me wonder if this specific code-path has been tested ? Please make sure to test this specific code-path.
Regards,
Hans
>
> ---
> Based on the latest platform-drivers-x86.git/for-next
> ---
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> index cffc1c9ba3e7..b19644ed12e0 100644
> --- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> +++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> @@ -258,13 +258,11 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> eloc++;
> break;
> case ORD_LIST_ELEMENTS:
> - size = ordered_list_data->elements_size;
> -
> /*
> * Ordered list data is stored in hex and comma separated format
> * Convert the data and split it to show each element
> */
> - ret = hp_convert_hexstr_to_str(str_value, value_len, &tmpstr, &tmp_len);
> + ret = hp_convert_hexstr_to_str(str_value, strlen(str_value), &tmpstr, &tmp_len);
> if (ret)
> goto exit_list;
>
> @@ -279,7 +277,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
> strscpy(ordered_list_data->elements[olist_elem],
> part,
> sizeof(ordered_list_data->elements[olist_elem]));
> - part = strsep(&part_tmp, SEMICOLON_SEP);
> + part = strsep(&part_tmp, COMMA_SEP);
> }
>
> kfree(str_value);
