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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 8 Aug 2023 23:51:59 +0200
From:   Hans de Goede <hdegoede@...hat.com>
To:     Jorge Lopez <jorgealtxwork@...il.com>
Cc:     platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
        thomas@...ch.de, ilpo.jarvinen@...ux.intel.com,
        dan.carpenter@...aro.org
Subject: Re: [PATCH] hp-bioscfg: Update string length calculation

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@...hat.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