Replace method how the string length is calculated.
Removed unused variable 'size'
Signed-off-by: Jorge Lopez <[email protected]>
---
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);
--
2.34.1
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 <[email protected]>
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);
Hi Hans,
On Mon, Aug 7, 2023 at 6:28 AM Hans de Goede <[email protected]> wrote:
>
> 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 <[email protected]>
>
> 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.
I will investigate.
>
> 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.
>
I will investigate and make the necessary corrections.
> 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.
ordered_list_data->elements_size is checked against MAX_ELEMENTS_SIZE
and not against the number of elements in the array. Initially, size
value was reported (sysfs) but after a couple reviews, size was
removed from being reported (sysfs). size value will be determined by
the application when it enumerates the values reported in elements.
>
> 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.
The purpose for 'eloc' is to help skip reading values such
ORD_LIST_ELEMENTS and PREREQUISITES when ORD_LIST_ELEMENTS and/or
PREREQUISITES_SIZE values are zero.
Normally, 'eloc' and 'elem' are the same.
>
> This all makes me wonder if this specific code-path has been tested ? Please make sure to test this specific code-path.
>
This code path was tested previously. I will make sure the path is
tested in deeper detail.
> 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);
>
Hi,
On 8/8/23 22:25, Jorge Lopez wrote:
> Hi Hans,
>
> On Mon, Aug 7, 2023 at 6:28 AM Hans de Goede <[email protected]> wrote:
<snip>
>> 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.
>
> ordered_list_data->elements_size is checked against MAX_ELEMENTS_SIZE
> and not against the number of elements in the array. Initially, size
> value was reported (sysfs) but after a couple reviews, size was
> removed from being reported (sysfs). size value will be determined by
> the application when it enumerates the values reported in elements.
Right, but after splitting the string on commas there should be ordered_list_data->elements_size entries, right ? So we should verify that. Also what if the string after splitting has more entries then MAX_ELEMENTS_SIZE, then currently the code will overflow the array, so the loop splitting the string on commas should ensure that MAX_ELEMENTS_SIZE is not exceeded.
>>
>> 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.
>
> The purpose for 'eloc' is to help skip reading values such
> ORD_LIST_ELEMENTS and PREREQUISITES when ORD_LIST_ELEMENTS and/or
> PREREQUISITES_SIZE values are zero.
> Normally, 'eloc' and 'elem' are the same.
Never mind what I meant to say is that you should to a check like this:
/* Check that both expected and read object type match */
if (expected_order_types[eloc] != order_obj[elem].type) {
pr_err("Error expected type %d for elem %d, but got type %d instead\n"
expected_order_types[eloc], elem, order_obj[elem].type);
return -EIO;
}
But I see now that that check is already there.
Regards,
Hans
On Tue, Aug 8, 2023 at 4:52 PM Hans de Goede <[email protected]> wrote:
>
> Hi,
>
> On 8/8/23 22:25, Jorge Lopez wrote:
> > Hi Hans,
> >
> > On Mon, Aug 7, 2023 at 6:28 AM Hans de Goede <[email protected]> wrote:
>
> <snip>
>
> >> 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.
> >
> > ordered_list_data->elements_size is checked against MAX_ELEMENTS_SIZE
> > and not against the number of elements in the array. Initially, size
> > value was reported (sysfs) but after a couple reviews, size was
> > removed from being reported (sysfs). size value will be determined by
> > the application when it enumerates the values reported in elements.
>
> Right, but after splitting the string on commas there should be ordered_list_data->elements_size entries, right ? So we should verify that. Also what if the string after splitting has more entries then MAX_ELEMENTS_SIZE, then currently the code will overflow the array, so the loop splitting the string on commas should ensure that MAX_ELEMENTS_SIZE is not exceeded.
That is correct. It is expected for the element size value to be 1
and does not represent the actual number of elements stored in comma
separated format. Element size value will be recalculated to report
the correct number of data elements present.
The loop restricts the max number of elements to MAX_ELEMENTS_SIZE to
avoid overflow the array..
I will make the necessary correction.
>
> >>
> >> 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.
> >
> > The purpose for 'eloc' is to help skip reading values such
> > ORD_LIST_ELEMENTS and PREREQUISITES when ORD_LIST_ELEMENTS and/or
> > PREREQUISITES_SIZE values are zero.
> > Normally, 'eloc' and 'elem' are the same.
>
> Never mind what I meant to say is that you should to a check like this:
>
> /* Check that both expected and read object type match */
> if (expected_order_types[eloc] != order_obj[elem].type) {
> pr_err("Error expected type %d for elem %d, but got type %d instead\n"
> expected_order_types[eloc], elem, order_obj[elem].type);
> return -EIO;
> }
>
> But I see now that that check is already there.
>
> Regards,
>
> Hans
>
>