[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2978ca9d-fbab-45b1-8d90-321fb9453965@kadam.mountain>
Date: Mon, 31 Jul 2023 19:16:35 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Jorge Lopez <jorgealtxwork@...il.com>
Cc: hdegoede@...hat.com, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org, thomas@...ch.de,
ilpo.jarvinen@...ux.intel.com
Subject: Re: [PATCH 2/5] hp-bioscfg: Fix memory leaks in
ordered_list_elements_from_package
On Mon, Jul 31, 2023 at 11:03:42AM -0500, Jorge Lopez wrote:
> On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@...aro.org> wrote:
> > 134 int value_len = 0;
> > 135 int ret;
> > 136 u32 size;
> > 137 u32 int_value = 0;
> >
> > It confused me that it's called int_value when it's not an int. Just
> > call it "u32 value = 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.
> > 201 case SEQUENCE:
> > 202 ordered_list_data->common.sequence = int_value;
> > 203 break;
> > 204 case PREREQUISITES_SIZE:
> > 205 ordered_list_data->common.prerequisites_size = int_value;
> > 206 if (int_value > MAX_PREREQUISITES_SIZE)
> > 207 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> >
> > ret = -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?
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 maximum number of elements supported or data may be malformed\n");
int_value = MAX_PREREQUISITES_SIZE;
}
ordered_list_data->common.prerequisites_size = int_value;
> > 257 case ORD_LIST_ELEMENTS:
> > 258 size = 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 show each element
> > 263 */
> > 264 ret = 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_ELEMENTS
> > or PREREQUISITES so this value_len comes from the PATH object.
> The size
?
regards,
dan carpenter
Powered by blists - more mailing lists