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

[Hans de Goede <hdegoede@redhat.com>]

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@redhat.com> 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