[PATCH 0/5] hp-bioscfg: Address memory leaks and uninitialized variable errors

[PATCH 0/5] hp-bioscfg: Address memory leaks and uninitialized variable errors

[Jorge Lopez]

Submit individual patches to address memory leaks and uninitialized 
variable errors for each source file listed below.

- hp_populate_string_elements_from_package()
drivers/platform/x86/hp/hp-bioscfg/string-attributes.c

- hp_populate_ordered_list_elements_from_package()
drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c

- hp_populate_integer_elements_from_package()
drivers/platform/x86/hp/hp-bioscfg/int-attributes.c

- hp_populate_enumeration_elements_from_package()
drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c

- hp_populate_password_elements_from_package()
drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c

Changes were tested with a HP EliteBook x360 1030 G3


Jorge Lopez (5):
  hp-bioscfg: Fix memory leaks in string_elements_from_package()
  hp-bioscfg: Fix memory leaks in ordered_list_elements_from_package
  hp-bioscfg: Fix memory leaks in integer_elements_from_package
  hp-bioscfg: Fix memory leaks in enumeration_elements_from_package()
  hp-bioscfg: Fix memory leaks in password_elements_from_package()

 drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c   |  8 +++++++-
 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c    |  7 ++++++-
 .../platform/x86/hp/hp-bioscfg/order-list-attributes.c | 10 ++++++++--
 .../platform/x86/hp/hp-bioscfg/passwdobj-attributes.c  | 10 +++++++++-
 drivers/platform/x86/hp/hp-bioscfg/string-attributes.c |  5 ++++-
 5 files changed, 34 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/5] hp-bioscfg: Fix memory leaks in string_elements_from_package()

[Jorge Lopez]

Address memory leaks in hp_populate_string_elements_from_package()
and uninitialized variable errors.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 drivers/platform/x86/hp/hp-bioscfg/string-attributes.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
index 1b62e372fb9e..e0ecdfca4def 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
@@ -133,7 +133,7 @@ static int hp_populate_string_elements_from_package(union acpi_object *string_ob
 	char *str_value = NULL;
 	int value_len;
 	int ret = 0;
-	u32 int_value;
+	u32 int_value = 0;
 	int elem;
 	int reqs;
 	int eloc;
@@ -171,6 +171,7 @@ static int hp_populate_string_elements_from_package(union acpi_object *string_ob
 		if (expected_string_types[eloc] != string_obj[elem].type) {
 			pr_err("Error expected type %d for elem %d, but got type %d instead\n",
 			       expected_string_types[eloc], elem, string_obj[elem].type);
+			kfree(str_value);
 			return -EIO;
 		}
 
@@ -232,6 +233,7 @@ static int hp_populate_string_elements_from_package(union acpi_object *string_ob
 					str_value,
 					sizeof(string_data->common.prerequisites[reqs]));
 				kfree(str_value);
+				str_value = NULL;
 			}
 			break;
 
@@ -250,6 +252,7 @@ static int hp_populate_string_elements_from_package(union acpi_object *string_ob
 		}
 
 		kfree(str_value);
+		str_value = NULL;
 	}
 
 exit_string_package:
-- 
2.34.1

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

[Jorge Lopez]

Address memory leaks in hp_populate_ordered_list_elements_from_package() and
uninitialized variable errors.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 .../platform/x86/hp/hp-bioscfg/order-list-attributes.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
index 7e49a8427c06..89e67db733eb 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
@@ -131,10 +131,10 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
 							  int instance_id)
 {
 	char *str_value = NULL;
-	int value_len;
+	int value_len = 0;
 	int ret;
 	u32 size;
-	u32 int_value;
+	u32 int_value = 0;
 	int elem;
 	int reqs;
 	int eloc;
@@ -174,6 +174,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
 		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);
+			kfree(str_value);
 			return -EIO;
 		}
 
@@ -231,6 +232,7 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
 					sizeof(ordered_list_data->common.prerequisites[reqs]));
 
 				kfree(str_value);
+				str_value = NULL;
 			}
 			break;
 
@@ -277,13 +279,17 @@ static int hp_populate_ordered_list_elements_from_package(union acpi_object *ord
 				part = strsep(&part_tmp, SEMICOLON_SEP);
 			}
 
+			kfree(str_value);
+			str_value = NULL;
 			break;
 		default:
 			pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem);
 			break;
 		}
 		kfree(tmpstr);
+		tmpstr = NULL;
 		kfree(str_value);
+		str_value = NULL;
 	}
 
 exit_list:
-- 
2.34.1

[PATCH 3/5] hp-bioscfg: Fix memory leaks in integer_elements_from_package

[Jorge Lopez]

Address memory leaks in hp_populate_integer_elements_from_package()
and uninitialized variable errors.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 drivers/platform/x86/hp/hp-bioscfg/int-attributes.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
index 7364c5ef9ef8..97f88e0ef0cc 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
@@ -143,7 +143,7 @@ static int hp_populate_integer_elements_from_package(union acpi_object *integer_
 	char *str_value = NULL;
 	int value_len;
 	int ret;
-	u32 int_value;
+	u32 int_value = 0;
 	int elem;
 	int reqs;
 	int eloc;
@@ -179,6 +179,7 @@ static int hp_populate_integer_elements_from_package(union acpi_object *integer_
 		if (expected_integer_types[eloc] != integer_obj[elem].type) {
 			pr_err("Error expected type %d for elem %d, but got type %d instead\n",
 			       expected_integer_types[eloc], elem, integer_obj[elem].type);
+			kfree(str_value);
 			return -EIO;
 		}
 		/* Assign appropriate element value to corresponding field*/
@@ -239,6 +240,7 @@ static int hp_populate_integer_elements_from_package(union acpi_object *integer_
 					str_value,
 					sizeof(integer_data->common.prerequisites[reqs]));
 				kfree(str_value);
+				str_value = NULL;
 			}
 			break;
 
@@ -258,6 +260,9 @@ static int hp_populate_integer_elements_from_package(union acpi_object *integer_
 			pr_warn("Invalid element: %d found in Integer attribute or data may be malformed\n", elem);
 			break;
 		}
+
+		kfree(str_value);
+		str_value = NULL;
 	}
 exit_integer_package:
 	kfree(str_value);
-- 
2.34.1

[PATCH 4/5] hp-bioscfg: Fix memory leaks in enumeration_elements_from_package()

[Jorge Lopez]

Address memory leaks in hp_populate_enumeration_elements_from_package()
and uninitialized variable errors.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
index b1b241f0205a..7f77963cd7fa 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
@@ -129,7 +129,7 @@ static int hp_populate_enumeration_elements_from_package(union acpi_object *enum
 	char *str_value = NULL;
 	int value_len;
 	u32 size = 0;
-	u32 int_value;
+	u32 int_value = 0;
 	int elem = 0;
 	int reqs;
 	int pos_values;
@@ -164,6 +164,7 @@ static int hp_populate_enumeration_elements_from_package(union acpi_object *enum
 		if (expected_enum_types[eloc] != enum_obj[elem].type) {
 			pr_err("Error expected type %d for elem %d, but got type %d instead\n",
 			       expected_enum_types[eloc], elem, enum_obj[elem].type);
+			kfree(str_value);
 			return -EIO;
 		}
 
@@ -224,6 +225,7 @@ static int hp_populate_enumeration_elements_from_package(union acpi_object *enum
 					sizeof(enum_data->common.prerequisites[reqs]));
 
 				kfree(str_value);
+				str_value = NULL;
 			}
 			break;
 
@@ -275,6 +277,9 @@ static int hp_populate_enumeration_elements_from_package(union acpi_object *enum
 					strscpy(enum_data->possible_values[pos_values],
 						str_value,
 						sizeof(enum_data->possible_values[pos_values]));
+
+				kfree(str_value);
+				str_value = NULL;
 			}
 			break;
 		default:
@@ -283,6 +288,7 @@ static int hp_populate_enumeration_elements_from_package(union acpi_object *enum
 		}
 
 		kfree(str_value);
+		str_value = NULL;
 	}
 
 exit_enumeration_package:
-- 
2.34.1

[PATCH 5/5] hp-bioscfg: Fix memory leaks in password_elements_from_package()

[Jorge Lopez]

Address memory leaks in hp_populate_password_elements_from_package()
and uninitialized variable errors.

Signed-off-by: Jorge Lopez <jorge.lopez2@hp.com>

---
Based on the latest platform-drivers-x86.git/for-next
---
 .../platform/x86/hp/hp-bioscfg/passwdobj-attributes.c  | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
index 52e4d346b529..5e833ea0c5e3 100644
--- a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
+++ b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
@@ -227,7 +227,7 @@ static int hp_populate_password_elements_from_package(union acpi_object *passwor
 	int value_len;
 	int ret;
 	u32 size;
-	u32 int_value;
+	u32 int_value = 0;
 	int elem;
 	int reqs;
 	int eloc;
@@ -264,6 +264,7 @@ static int hp_populate_password_elements_from_package(union acpi_object *passwor
 		if (expected_password_types[eloc] != password_obj[elem].type) {
 			pr_err("Error expected type %d for elem %d, but got type %d instead\n",
 			       expected_password_types[eloc], elem, password_obj[elem].type);
+			kfree(str_value);
 			return -EIO;
 		}
 
@@ -318,6 +319,8 @@ static int hp_populate_password_elements_from_package(union acpi_object *passwor
 					sizeof(password_data->common.prerequisites[reqs]));
 
 				kfree(str_value);
+				str_value = NULL;
+
 			}
 			break;
 		case SECURITY_LEVEL:
@@ -356,6 +359,8 @@ static int hp_populate_password_elements_from_package(union acpi_object *passwor
 					str_value,
 					sizeof(password_data->encodings[pos_values]));
 				kfree(str_value);
+				str_value = NULL;
+
 			}
 			break;
 		case PSWD_IS_SET:
@@ -365,6 +370,9 @@ static int hp_populate_password_elements_from_package(union acpi_object *passwor
 			pr_warn("Invalid element: %d found in Password attribute or data may be malformed\n", elem);
 			break;
 		}
+
+		kfree(str_value);
+		str_value = NULL;
 	}
 
 exit_package:
-- 
2.34.1

Re: [PATCH 0/5] hp-bioscfg: Address memory leaks and uninitialized variable errors

[Hans de Goede]

Hi Jorge,

On 7/26/23 00:00, Jorge Lopez wrote:
> Submit individual patches to address memory leaks and uninitialized 
> variable errors for each source file listed below.
> 
> - hp_populate_string_elements_from_package()
> drivers/platform/x86/hp/hp-bioscfg/string-attributes.c
> 
> - hp_populate_ordered_list_elements_from_package()
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
> 
> - hp_populate_integer_elements_from_package()
> drivers/platform/x86/hp/hp-bioscfg/int-attributes.c
> 
> - hp_populate_enumeration_elements_from_package()
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
> 
> - hp_populate_password_elements_from_package()
> drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
> 
> Changes were tested with a HP EliteBook x360 1030 G3

Thank you for the patches fixing this.

I'm going to wait a bit with applying these to give other people who have looked at these issues a chance to review these patches (added Dan Carpenter and Christophe JAILLET to the To: list).

For future patches please write one patch for each issue you are addressing instead of one patch per file. Since the fixes are the same for all 5 files in this case that would have meant doing 2 patches:

1. To fix the memory leak errors in all 5 files
2. To fix the uninitialized variable in all 5 files.

There is no need to send a new version unless changes are requested by one of the reviewers, I'll just squash all these 5 patches into 1 patch when merging this (and keep the uninitialized variable changes as part of the same patch).

Also these patches should have something like this as part of the commit msg (directly above your Signed-off-by:) :

Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/platform-driver-x86/176e14a2-41f8-41b2-854e-77319ec2416d@moroto.mountain/

Regards,

Hans








> 
> 
> Jorge Lopez (5):
>   hp-bioscfg: Fix memory leaks in string_elements_from_package()
>   hp-bioscfg: Fix memory leaks in ordered_list_elements_from_package
>   hp-bioscfg: Fix memory leaks in integer_elements_from_package
>   hp-bioscfg: Fix memory leaks in enumeration_elements_from_package()
>   hp-bioscfg: Fix memory leaks in password_elements_from_package()
> 
>  drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c   |  8 +++++++-
>  drivers/platform/x86/hp/hp-bioscfg/int-attributes.c    |  7 ++++++-
>  .../platform/x86/hp/hp-bioscfg/order-list-attributes.c | 10 ++++++++--
>  .../platform/x86/hp/hp-bioscfg/passwdobj-attributes.c  | 10 +++++++++-
>  drivers/platform/x86/hp/hp-bioscfg/string-attributes.c |  5 ++++-
>  5 files changed, 34 insertions(+), 6 deletions(-)
> 

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

[Dan Carpenter]

I went through this pretty carefully.  There is a small bug in the
ORD_LIST_ELEMENTS case where the value_len is wrong.  Otherwise, I
complained a little about style nits...  You can feel free to ignore
everything except the ORD_LIST_ELEMENTS stuff.

regards,
dan carpenter

drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
   129  static int hp_populate_ordered_list_elements_from_package(union acpi_object *order_obj,
   130                                                            int order_obj_count,
   131                                                            int instance_id)
   132  {
   133          char *str_value = NULL;

It would be better to make str_value local to the loop.  Then we
could delete all the str_value = NULL assignments and the exit_list:
code at the end.  At the end of the loop we could do something like:

	kfree(str_value);
	if (ret)
		return ret;

   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;".

   138          int elem;
   139          int reqs;
   140          int eloc;
   141          char *tmpstr = NULL;
   142          char *part_tmp = NULL;
   143          int tmp_len = 0;
   144          char *part = NULL;
   145          struct ordered_list_data *ordered_list_data = &bioscfg_drv.ordered_list_data[instance_id];
   146  
   147          if (!order_obj)
   148                  return -EINVAL;
   149  
   150          for (elem = 1, eloc = 1; elem < order_obj_count; elem++, eloc++) {
   151                  /* ONLY look at the first ORDERED_ELEM_CNT elements */
   152                  if (eloc == ORD_ELEM_CNT)
   153                          goto exit_list;

We really expect to exit when eloc is ORD_ELEM_CNT.  So I think this
would be more clear as:

	for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++) {

I don't really know what eloc stands for...  dst_idx?

		if (elem >= order_obj_count)
			return -EINVAL;

		obj = &order_obj[elem];

Let's move the "Check that both expected and read object type match"
stuff from line 173 up to here.

		if (obj->type != expected_order_types[eloc]) {
			pr_err("Error expected type %d for elem %d, but got type %d instead\n",
				expected_order_types[eloc], elem, obj->type);
			return -EINVAL;
		}

   154  
   155                  switch (order_obj[elem].type) {

	switch(obj->type) {

   156                  case ACPI_TYPE_STRING:
   157                          if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
   158                                  ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer,
   159                                                                 order_obj[elem].string.length,
   160                                                                 &str_value, &value_len);
   161                                  if (ret)
   162                                          continue;

return ret;

   163                          }
   164                          break;
   165                  case ACPI_TYPE_INTEGER:
   166                          int_value = (u32)order_obj[elem].integer.value;
   167                          break;
   168                  default:
   169                          pr_warn("Unsupported object type [%d]\n", order_obj[elem].type);
   170                          continue;

return -EINVAL;

   171                  }
   172  
   173                  /* Check that both expected and read object type match */
   174                  if (expected_order_types[eloc] != order_obj[elem].type) {
   175                          pr_err("Error expected type %d for elem %d, but got type %d instead\n",
   176                                 expected_order_types[eloc], elem, order_obj[elem].type);
   177                          kfree(str_value);
   178                          return -EIO;
   179                  }
   180  
   181                  /* Assign appropriate element value to corresponding field*/
   182                  switch (eloc) {
   183                  case VALUE:
   184                          strscpy(ordered_list_data->current_value,
   185                                  str_value, sizeof(ordered_list_data->current_value));
   186                          replace_char_str(ordered_list_data->current_value, COMMA_SEP, SEMICOLON_SEP);
   187                          break;
   188                  case PATH:
   189                          strscpy(ordered_list_data->common.path, str_value,
   190                                  sizeof(ordered_list_data->common.path));
   191                          break;
   192                  case IS_READONLY:
   193                          ordered_list_data->common.is_readonly = int_value;
   194                          break;
   195                  case DISPLAY_IN_UI:
   196                          ordered_list_data->common.display_in_ui = int_value;
   197                          break;
   198                  case REQUIRES_PHYSICAL_PRESENCE:
   199                          ordered_list_data->common.requires_physical_presence = int_value;
   200                          break;
   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;

   208  
   209                          /*
   210                           * This HACK is needed to keep the expected
   211                           * element list pointing to the right obj[elem].type
   212                           * when the size is zero. PREREQUISITES
   213                           * object is omitted by BIOS when the size is
   214                           * zero.

It's not really a HACK.

	/*
	 * If prerequisites_size is zero then there isn't a PREREQUISITES
	 * object so skip that and jump to SECURITY_LEVEL.
	 *
	 */


   215                           */
   216                          if (int_value == 0)
   217                                  eloc++;
   218                          break;
   219                  case PREREQUISITES:
   220                          size = min_t(u32, ordered_list_data->common.prerequisites_size,
   221                                       MAX_PREREQUISITES_SIZE);

If we returned when we hit invalid data then there is no need for this
min_t().

	size = ordered_list_data->common.prerequisites_size;

   222                          for (reqs = 0; reqs < size; reqs++) {
   223                                  ret = hp_convert_hexstr_to_str(order_obj[elem + reqs].string.pointer,
   224                                                                 order_obj[elem + reqs].string.length,
   225                                                                 &str_value, &value_len);

This is fine but it might be nicer to do what ORD_LIST_ELEMENTS does
and use tmpstr instead of str_value.

   226  
   227                                  if (ret)
   228                                          continue;

break;

   229  
   230                                  strscpy(ordered_list_data->common.prerequisites[reqs],
   231                                          str_value,
   232                                          sizeof(ordered_list_data->common.prerequisites[reqs]));
   233  
   234                                  kfree(str_value);
   235                                  str_value = NULL;
   236                          }
   237                          break;
   238  
   239                  case SECURITY_LEVEL:
   240                          ordered_list_data->common.security_level = int_value;
   241                          break;
   242  
   243                  case ORD_LIST_SIZE:
   244                          ordered_list_data->elements_size = int_value;
   245                          if (int_value > MAX_ELEMENTS_SIZE)
   246                                  pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n");

ret = -EINVAL;
break;

   247                          /*
   248                           * This HACK is needed to keep the expected
   249                           * element list pointing to the right obj[elem].type
   250                           * when the size is zero. ORD_LIST_ELEMENTS
   251                           * object is omitted by BIOS when the size is
   252                           * zero.
   253                           */
   254                          if (int_value == 0)
   255                                  eloc++;
   256                          break;
   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.

   265                          if (ret)
   266                                  goto exit_list;
   267  
   268                          part_tmp = tmpstr;
   269                          part = strsep(&part_tmp, COMMA_SEP);
   270                          if (!part)
   271                                  strscpy(ordered_list_data->elements[0],
   272                                          tmpstr,
   273                                          sizeof(ordered_list_data->elements[0]));
   274  
   275                          for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) {

Please don't re-use the "elem" iterator for this inner loop.

regards,
dan carpenter

   276                                  strscpy(ordered_list_data->elements[elem],
   277                                          part,
   278                                          sizeof(ordered_list_data->elements[elem]));
   279                                  part = strsep(&part_tmp, SEMICOLON_SEP);
   280                          }
   281
   282                          kfree(str_value);
   283                          str_value = NULL;
   284                          break;
   285                  default:
   286                          pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem);
   287                          break;
   288                  }
   289                  kfree(tmpstr);
   290                  tmpstr = NULL;
   291                  kfree(str_value);
   292                  str_value = NULL;
   293          }
   294
   295  exit_list:
   296          kfree(tmpstr);
   297          kfree(str_value);
   298          return 0;
   299  }

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

[Jorge Lopez]

On Thu, Jul 27, 2023 at 1:21 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> I went through this pretty carefully.  There is a small bug in the
> ORD_LIST_ELEMENTS case where the value_len is wrong.  Otherwise, I
> complained a little about style nits...  You can feel free to ignore
> everything except the ORD_LIST_ELEMENTS stuff.
>
> regards,
> dan carpenter
>
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c
>    129  static int hp_populate_ordered_list_elements_from_package(union acpi_object *order_obj,
>    130                                                            int order_obj_count,
>    131                                                            int instance_id)
>    132  {
>    133          char *str_value = NULL;
>
> It would be better to make str_value local to the loop.  Then we
> could delete all the str_value = NULL assignments and the exit_list:
> code at the end.  At the end of the loop we could do something like:
>
>         kfree(str_value);
>         if (ret)
>                 return ret;
>
>    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;
>
>    138          int elem;
>    139          int reqs;
>    140          int eloc;
>    141          char *tmpstr = NULL;
>    142          char *part_tmp = NULL;
>    143          int tmp_len = 0;
>    144          char *part = NULL;
>    145          struct ordered_list_data *ordered_list_data = &bioscfg_drv.ordered_list_data[instance_id];
>    146
>    147          if (!order_obj)
>    148                  return -EINVAL;
>    149
>    150          for (elem = 1, eloc = 1; elem < order_obj_count; elem++, eloc++) {
>    151                  /* ONLY look at the first ORDERED_ELEM_CNT elements */
>    152                  if (eloc == ORD_ELEM_CNT)
>    153                          goto exit_list;
>
> We really expect to exit when eloc is ORD_ELEM_CNT.  So I think this
> would be more clear as:
>
>         for (elem = 1, eloc = 1; eloc < ORD_ELEM_CNT; elem++, eloc++) {
>
Done!

> I don't really know what eloc stands for...  dst_idx?
>
>                 if (elem >= order_obj_count)
>                         return -EINVAL;
>
>                 obj = &order_obj[elem];
>

'eloc' stands for 'element location'.  eloc helps keep track
conditions such when ORD_LIST_SIZE and/or PREREQUISITES_SiZE value is
zero.


> Let's move the "Check that both expected and read object type match"
> stuff from line 173 up to here.
>
>                 if (obj->type != expected_order_types[eloc]) {
>                         pr_err("Error expected type %d for elem %d, but got type %d instead\n",
>                                 expected_order_types[eloc], elem, obj->type);
>                         return -EINVAL;
>                 }
>
>    154
>    155                  switch (order_obj[elem].type) {
>
>         switch(obj->type) {
>
>    156                  case ACPI_TYPE_STRING:
>    157                          if (elem != PREREQUISITES && elem != ORD_LIST_ELEMENTS) {
>    158                                  ret = hp_convert_hexstr_to_str(order_obj[elem].string.pointer,
>    159                                                                 order_obj[elem].string.length,
>    160                                                                 &str_value, &value_len);
>    161                                  if (ret)
>    162                                          continue;
>
> return ret;
>
>    163                          }
>    164                          break;
>    165                  case ACPI_TYPE_INTEGER:
>    166                          int_value = (u32)order_obj[elem].integer.value;
>    167                          break;
>    168                  default:
>    169                          pr_warn("Unsupported object type [%d]\n", order_obj[elem].type);
>    170                          continue;
>
> return -EINVAL;

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.
>
>    171                  }
>    172
>    173                  /* Check that both expected and read object type match */
>    174                  if (expected_order_types[eloc] != order_obj[elem].type) {
>    175                          pr_err("Error expected type %d for elem %d, but got type %d instead\n",
>    176                                 expected_order_types[eloc], elem, order_obj[elem].type);
>    177                          kfree(str_value);
>    178                          return -EIO;
>    179                  }
>    180
>    181                  /* Assign appropriate element value to corresponding field*/
>    182                  switch (eloc) {
>    183                  case VALUE:
>    184                          strscpy(ordered_list_data->current_value,
>    185                                  str_value, sizeof(ordered_list_data->current_value));
>    186                          replace_char_str(ordered_list_data->current_value, COMMA_SEP, SEMICOLON_SEP);
>    187                          break;
>    188                  case PATH:
>    189                          strscpy(ordered_list_data->common.path, str_value,
>    190                                  sizeof(ordered_list_data->common.path));
>    191                          break;
>    192                  case IS_READONLY:
>    193                          ordered_list_data->common.is_readonly = int_value;
>    194                          break;
>    195                  case DISPLAY_IN_UI:
>    196                          ordered_list_data->common.display_in_ui = int_value;
>    197                          break;
>    198                  case REQUIRES_PHYSICAL_PRESENCE:
>    199                          ordered_list_data->common.requires_physical_presence = int_value;
>    200                          break;
>    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.

>    208
>    209                          /*
>    210                           * This HACK is needed to keep the expected
>    211                           * element list pointing to the right obj[elem].type
>    212                           * when the size is zero. PREREQUISITES
>    213                           * object is omitted by BIOS when the size is
>    214                           * zero.
>
> It's not really a HACK.

Will change the term HACK  to 'step'
>
>         /*
>          * If prerequisites_size is zero then there isn't a PREREQUISITES
>          * object so skip that and jump to SECURITY_LEVEL.
>          *
>          */
>
>
>    215                           */
>    216                          if (int_value == 0)
>    217                                  eloc++;
>    218                          break;
>    219                  case PREREQUISITES:
>    220                          size = min_t(u32, ordered_list_data->common.prerequisites_size,
>    221                                       MAX_PREREQUISITES_SIZE);
>
> If we returned when we hit invalid data then there is no need for this
> min_t().
>
>         size = ordered_list_data->common.prerequisites_size;
>
>    222                          for (reqs = 0; reqs < size; reqs++) {
>    223                                  ret = hp_convert_hexstr_to_str(order_obj[elem + reqs].string.pointer,
>    224                                                                 order_obj[elem + reqs].string.length,
>    225                                                                 &str_value, &value_len);
>
> This is fine but it might be nicer to do what ORD_LIST_ELEMENTS does
> and use tmpstr instead of str_value.
>
>    226
>    227                                  if (ret)
>    228                                          continue;
>
> break;
>
>    229
>    230                                  strscpy(ordered_list_data->common.prerequisites[reqs],
>    231                                          str_value,
>    232                                          sizeof(ordered_list_data->common.prerequisites[reqs]));
>    233
>    234                                  kfree(str_value);
>    235                                  str_value = NULL;
>    236                          }
>    237                          break;
>    238
>    239                  case SECURITY_LEVEL:
>    240                          ordered_list_data->common.security_level = int_value;
>    241                          break;
>    242
>    243                  case ORD_LIST_SIZE:
>    244                          ordered_list_data->elements_size = int_value;
>    245                          if (int_value > MAX_ELEMENTS_SIZE)
>    246                                  pr_warn("Ordered List size value exceeded the maximum number of elements supported or data may be malformed\n");
>
> ret = -EINVAL;
> break;
>
>    247                          /*
>    248                           * This HACK is needed to keep the expected
>    249                           * element list pointing to the right obj[elem].type
>    250                           * when the size is zero. ORD_LIST_ELEMENTS
>    251                           * object is omitted by BIOS when the size is
>    252                           * zero.
>    253                           */
>    254                          if (int_value == 0)
>    255                                  eloc++;
>    256                          break;
>    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
>
>    265                          if (ret)
>    266                                  goto exit_list;
>    267
>    268                          part_tmp = tmpstr;
>    269                          part = strsep(&part_tmp, COMMA_SEP);
>    270                          if (!part)
>    271                                  strscpy(ordered_list_data->elements[0],
>    272                                          tmpstr,
>    273                                          sizeof(ordered_list_data->elements[0]));
>    274
>    275                          for (elem = 1; elem < MAX_ELEMENTS_SIZE && part; elem++) {
>
> Please don't re-use the "elem" iterator for this inner loop.
Done!
>
> regards,
> dan carpenter
>
>    276                                  strscpy(ordered_list_data->elements[elem],
>    277                                          part,
>    278                                          sizeof(ordered_list_data->elements[elem]));
>    279                                  part = strsep(&part_tmp, SEMICOLON_SEP);
>    280                          }
>    281
>    282                          kfree(str_value);
>    283                          str_value = NULL;
>    284                          break;
>    285                  default:
>    286                          pr_warn("Invalid element: %d found in Ordered_List attribute or data may be malformed\n", elem);
>    287                          break;
>    288                  }
>    289                  kfree(tmpstr);
>    290                  tmpstr = NULL;
>    291                  kfree(str_value);
>    292                  str_value = NULL;
>    293          }
>    294
>    295  exit_list:
>    296          kfree(tmpstr);
>    297          kfree(str_value);
>    298          return 0;
>    299  }
>
>

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@linaro.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

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

[Jorge Lopez]

On Mon, Jul 31, 2023 at 11:16 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> 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@linaro.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.

Oops!  just send a new patch using type_int_value.  Should I change it back?
>
> > >    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?

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 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
>
I meant to say,  I should have used 'size' instead of 'value_len' in line 264.

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

> ?
>
> regards,
> dan carpenter

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

[Dan Carpenter]

On Mon, Jul 31, 2023 at 11:35:35AM -0500, Jorge Lopez wrote:
> On Mon, Jul 31, 2023 at 11:16 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > 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@linaro.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.
> 
> Oops!  just send a new patch using type_int_value.  Should I change it back?

In order of preference for me, it's "value", "int_value", and
"type_int_value".  But it doesn't really matter.  I feel there were a
couple bugs like the size vs value_len and re-using the iterator.  You
have addressed the real problems so lets not worry about variable names.
Whatever you pick is fine.

regards,
dan carpenter

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

[Jorge Lopez]

I submitted a new set of patches as directed by Hans in which the
variable name 'int_value' was kept.
Thank you for replying back to me.

Regards,

Jorge Lopez

On Tue, Aug 1, 2023 at 12:54 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Mon, Jul 31, 2023 at 11:35:35AM -0500, Jorge Lopez wrote:
> > On Mon, Jul 31, 2023 at 11:16 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > 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@linaro.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.
> >
> > Oops!  just send a new patch using type_int_value.  Should I change it back?
>
> In order of preference for me, it's "value", "int_value", and
> "type_int_value".  But it doesn't really matter.  I feel there were a
> couple bugs like the size vs value_len and re-using the iterator.  You
> have addressed the real problems so lets not worry about variable names.
> Whatever you pick is fine.
>
> regards,
> dan carpenter
>