Messages in this thread | | | Date | Tue, 8 Aug 2023 23:51:59 +0200 | Subject | Re: [PATCH] hp-bioscfg: Update string length calculation | From | Hans de Goede <> |
| |
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
| |