Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1ea7e949-e05b-985c-bb0c-6d0b00bb8b5f@redhat.com>
Date:   Tue, 8 Aug 2023 23:51:59 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] hp-bioscfg: Update string length calculation
To:     Jorge Lopez <jorgealtxwork@gmail.com>
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org,
        thomas@t-8ch.de, ilpo.jarvinen@linux.intel.com,
        dan.carpenter@linaro.org
References: <20230801191629.45942-1-jorge.lopez2@hp.com>
 <4ab55129-d35c-fea5-0c59-5183928d55d1@redhat.com>
 <CAOOmCE_DGqUh3+8LmrX2_7eTeDG_7p7-JKN7Q2syvDQu60UWmQ@mail.gmail.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <CAOOmCE_DGqUh3+8LmrX2_7eTeDG_7p7-JKN7Q2syvDQu60UWmQ@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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