Re: [PATCH 2/5] hp-bioscfg: Fix memory leaks in ordered_list_elements_from_package

[Dan Carpenter]

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@xxxxxxxxxx> 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