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

[Jorge Lopez]

On Tue, Aug 8, 2023 at 4:52 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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 <hdegoede@xxxxxxxxxx> 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
>
>