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: <20230725220056.25560-1-jorge.lopez2@hp.com> <20230725220056.25560-3-jorge.lopez2@hp.com>
 <77867e93-7785-457c-9e37-4b41503bb509@moroto.mountain> <CAOOmCE_uRxqjTYueZkStbXeU5GKRUnvFOSGNhiBbtWDfkvxveg@mail.gmail.com>
 <2978ca9d-fbab-45b1-8d90-321fb9453965@kadam.mountain>
In-Reply-To: <2978ca9d-fbab-45b1-8d90-321fb9453965@kadam.mountain>
From:   Jorge Lopez <jorgealtxwork@gmail.com>
Date:   Mon, 31 Jul 2023 11:35:35 -0500
Message-ID: <CAOOmCE_p_QgV0QJZKtYZD+t7Y9MzNOQ9sEkfSM88VoHDUCwdCw@mail.gmail.com>
Subject: Re: [PATCH 2/5] hp-bioscfg: Fix memory leaks in ordered_list_elements_from_package
To:     Dan Carpenter <dan.carpenter@linaro.org>
Cc:     hdegoede@redhat.com, platform-driver-x86@vger.kernel.org,
        linux-kernel@vger.kernel.org, thomas@t-8ch.de,
        ilpo.jarvinen@linux.intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jul 31, 2023 at 11:16=E2=80=AFAM Dan Carpenter <dan.carpenter@linar=
o.org> wrote:
>
> On Mon, Jul 31, 2023 at 11:03:42AM -0500, Jorge Lopez wrote:
> > On Thu, Jul 27, 2023 at 1:21=E2=80=AFAM Dan Carpenter <dan.carpenter@li=
naro.org> wrote:
> > >    134          int value_len =3D 0;
> > >    135          int ret;
> > >    136          u32 size;
> > >    137          u32 int_value =3D 0;
> > >
> > > It confused me that it's called int_value when it's not an int.  Just
> > > call it "u32 value =3D 0;".
> >
> > The variable is named int_value when it is not an int because it
> > stores the value reported by ACPI_TYPE_INTEGER package.
> > The variable will be renamed to  type_int_value;
>
> Eep!  That's even worse!  Just leave it as-is then.

Oops!  just send a new patch using type_int_value.  Should I change it back=
?
>
> > >    201                  case SEQUENCE:
> > >    202                          ordered_list_data->common.sequence =
=3D int_value;
> > >    203                          break;
> > >    204                  case PREREQUISITES_SIZE:
> > >    205                          ordered_list_data->common.prerequisit=
es_size =3D int_value;
> > >    206                          if (int_value > MAX_PREREQUISITES_SIZ=
E)
> > >    207                                  pr_warn("Prerequisites size v=
alue exceeded the maximum number of elements supported or data may be malfo=
rmed\n");
> > >
> > >         ret =3D -EINVAL;
> > >         break;
> > >
> > We encountered during our testing that one or more packages could be
> > assigned the wrong package type or invalid data..
> > For this reason, it was decided to ignore any invalid data and
> > incorrect type package and allow the read process to continue.
> >
>
> So you have BIOSes which are still printing this warning and you can't
> fix it?  Fine.  Are you sure it's not because you re-used the elem
> iterator and started looping again in the middle?

Yes.  I am sure.   The BIOS where this code is applicable to is for
2018 platforms and earlier.
This is the reason, a customer may encounter this problem in an old BIOS.

>
> Could you at least do the bounds check here instead of in the next step?
>
>
>                 if (int_value > MAX_PREREQUISITES_SIZE) {
>                         pr_warn("Prerequisites size value exceeded the ma=
ximum number of elements supported or data may be malformed\n");
>                         int_value =3D MAX_PREREQUISITES_SIZE;
>                 }
>                 ordered_list_data->common.prerequisites_size =3D int_valu=
e;
>
> > >    257                  case ORD_LIST_ELEMENTS:
> > >    258                          size =3D ordered_list_data->elements_=
size;
> > >
> > > We don't use size anywhere so this can be deleted.
> > >
> > >    259
> > >    260                          /*
> > >    261                           * Ordered list data is stored in hex=
 and comma separated format
> > >    262                           * Convert the data and split it to s=
how each element
> > >    263                           */
> > >    264                          ret =3D hp_convert_hexstr_to_str(str_=
value, value_len, &tmpstr, &tmp_len);
> > >
> > > The value_len here is wrong.  We don't read value_len for ORD_LIST_EL=
EMENTS
> > > or PREREQUISITES so this value_len comes from the PATH object.
> > The size
>
I meant to say,  I should have used 'size' instead of 'value_len' in line 2=
64.

ret =3D hp_convert_hexstr_to_str(str_value, size, &tmpstr, &tmp_len);

> ?
>
> regards,
> dan carpenter