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;
MIME-Version: 1.0
References: <20230801191629.45942-1-jorge.lopez2@hp.com> <4ab55129-d35c-fea5-0c59-5183928d55d1@redhat.com>
 <CAOOmCE_DGqUh3+8LmrX2_7eTeDG_7p7-JKN7Q2syvDQu60UWmQ@mail.gmail.com> <1ea7e949-e05b-985c-bb0c-6d0b00bb8b5f@redhat.com>
In-Reply-To: <1ea7e949-e05b-985c-bb0c-6d0b00bb8b5f@redhat.com>
From:   Jorge Lopez <jorgealtxwork@gmail.com>
Date:   Wed, 9 Aug 2023 14:59:07 -0500
Message-ID: <CAOOmCE9ms1iztr7pXUaEqEqr922NeTgcopPuWyC8s47A0HXb3w@mail.gmail.com>
Subject: Re: [PATCH] hp-bioscfg: Update string length calculation
To:     Hans de Goede <hdegoede@redhat.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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Aug 8, 2023 at 4:52=E2=80=AFPM Hans de Goede <hdegoede@redhat.com> =
wrote:
>
> Hi,
>
> On 8/8/23 22:25, Jorge Lopez wrote:
> > Hi Hans,
> >
> > On Mon, Aug 7, 2023 at 6:28=E2=80=AFAM Hans de Goede <hdegoede@redhat.c=
om> wrote:
>
> <snip>
>
> >> 3. ordered_list_data->elements_size is set but never validated. You sh=
ould 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_l=
ist_data->elements_size entries, right ? So we should verify that. Also wha=
t if the string after splitting has more entries then MAX_ELEMENTS_SIZE, th=
en currently the code will overflow the array, so the loop splitting the st=
ring 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. Pleas=
e 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] !=3D order_obj[elem].type)=
 {
>                         pr_err("Error expected type %d for elem %d, but g=
ot type %d instead\n"
>                                expected_order_types[eloc], elem, order_ob=
j[elem].type);
>                         return -EIO;
>                 }
>
> But I see now that that check is already there.
>
> Regards,
>
> Hans
>
>