Hi Hans,
On Mon, Aug 7, 2023 at 6:28 AM Hans de Goede <hdegoede@xxxxxxxxxx> 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 <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.
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);
>
