Re: [PATCH] hp-bioscfg: Update string length calculation

[Jorge Lopez]

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);
>