* [bug report] platform/x86: hp-bioscfg: enum-attributes
@ 2023-07-17 10:39 Dan Carpenter
2023-07-18 10:55 ` Ilpo Järvinen
2023-07-25 19:49 ` Lopez, Jorge A (Security)
0 siblings, 2 replies; 3+ messages in thread
From: Dan Carpenter @ 2023-07-17 10:39 UTC (permalink / raw)
To: jorge.lopez2; +Cc: platform-driver-x86
Hello Jorge Lopez,
The patch 6b2770bfd6f9: "platform/x86: hp-bioscfg: enum-attributes"
from Jun 8, 2023 (linux-next), leads to the following Smatch static
checker warning:
drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c:285 hp_populate_enumeration_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c:289 hp_populate_enumeration_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/int-attributes.c:263 hp_populate_integer_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:286 hp_populate_ordered_list_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:290 hp_populate_ordered_list_elements_from_package() error: double free of 'tmpstr'
drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:291 hp_populate_ordered_list_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c:371 hp_populate_password_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/string-attributes.c:252 hp_populate_string_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/string-attributes.c:256 hp_populate_string_elements_from_package() error: double free of 'str_value'
drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
125 static int hp_populate_enumeration_elements_from_package(union acpi_object *enum_obj,
126 int enum_obj_count,
127 int instance_id)
128 {
129 char *str_value = NULL;
str_value starts as NULL.
130 int value_len;
131 u32 size = 0;
132 u32 int_value;
133 int elem = 0;
134 int reqs;
135 int pos_values;
136 int ret;
137 int eloc;
138 struct enumeration_data *enum_data = &bioscfg_drv.enumeration_data[instance_id];
139
140 for (elem = 1, eloc = 1; elem < enum_obj_count; elem++, eloc++) {
141 /* ONLY look at the first ENUM_ELEM_CNT elements */
142 if (eloc == ENUM_ELEM_CNT)
143 goto exit_enumeration_package;
But here we free the str_value from the previous iteration.
144
145 switch (enum_obj[elem].type) {
146 case ACPI_TYPE_STRING:
147 if (PREREQUISITES != elem && ENUM_POSSIBLE_VALUES != elem) {
148 ret = hp_convert_hexstr_to_str(enum_obj[elem].string.pointer,
149 enum_obj[elem].string.length,
150 &str_value, &value_len);
151 if (ret)
152 return -EINVAL;
Here the str_value from the previos iteration is re-assigned without
being freed. (memory leak).
153 }
154 break;
155 case ACPI_TYPE_INTEGER:
156 int_value = (u32)enum_obj[elem].integer.value;
157 break;
158 default:
159 pr_warn("Unsupported object type [%d]\n", enum_obj[elem].type);
160 continue;
161 }
162
163 /* Check that both expected and read object type match */
164 if (expected_enum_types[eloc] != enum_obj[elem].type) {
165 pr_err("Error expected type %d for elem %d, but got type %d instead\n",
166 expected_enum_types[eloc], elem, enum_obj[elem].type);
167 return -EIO;
168 }
169
170 /* Assign appropriate element value to corresponding field */
171 switch (eloc) {
172 case NAME:
173 case VALUE:
174 break;
175 case PATH:
176 strscpy(enum_data->common.path, str_value,
If str_value is NULL this will crash.
177 sizeof(enum_data->common.path));
178 break;
179 case IS_READONLY:
180 enum_data->common.is_readonly = int_value;
181 break;
182 case DISPLAY_IN_UI:
183 enum_data->common.display_in_ui = int_value;
184 break;
185 case REQUIRES_PHYSICAL_PRESENCE:
186 enum_data->common.requires_physical_presence = int_value;
187 break;
188 case SEQUENCE:
189 enum_data->common.sequence = int_value;
190 break;
191 case PREREQUISITES_SIZE:
192 enum_data->common.prerequisites_size = int_value;
193 if (int_value > MAX_PREREQUISITES_SIZE)
194 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
195
196 /*
197 * This HACK is needed to keep the expected
198 * element list pointing to the right obj[elem].type
199 * when the size is zero. PREREQUISITES
200 * object is omitted by BIOS when the size is
201 * zero.
202 */
203 if (int_value == 0)
204 eloc++;
205 break;
206
207 case PREREQUISITES:
208 size = min_t(u32, enum_data->common.prerequisites_size, MAX_PREREQUISITES_SIZE);
209 for (reqs = 0; reqs < size; reqs++) {
210 if (elem >= enum_obj_count) {
211 pr_err("Error enum-objects package is too small\n");
212 return -EINVAL;
213 }
214
215 ret = hp_convert_hexstr_to_str(enum_obj[elem + reqs].string.pointer,
216 enum_obj[elem + reqs].string.length,
217 &str_value, &value_len);
str_value is re-assigned again. (memory leak).
218
219 if (ret)
220 return -EINVAL;
221
222 strscpy(enum_data->common.prerequisites[reqs],
223 str_value,
224 sizeof(enum_data->common.prerequisites[reqs]));
225
226 kfree(str_value);
str_value is freed. (this will lead to a crash).
227 }
228 break;
229
230 case SECURITY_LEVEL:
231 enum_data->common.security_level = int_value;
232 break;
233
234 case ENUM_CURRENT_VALUE:
235 strscpy(enum_data->current_value,
236 str_value, sizeof(enum_data->current_value));
No check for NULL.
237 break;
238 case ENUM_SIZE:
239 enum_data->possible_values_size = int_value;
240 if (int_value > MAX_VALUES_SIZE)
241 pr_warn("Possible number values size value exceeded the maximum number of elements supported or data may be malformed\n");
242
243 /*
244 * This HACK is needed to keep the expected
245 * element list pointing to the right obj[elem].type
246 * when the size is zero. POSSIBLE_VALUES
247 * object is omitted by BIOS when the size is zero.
248 */
249 if (int_value == 0)
250 eloc++;
251 break;
252
253 case ENUM_POSSIBLE_VALUES:
254 size = enum_data->possible_values_size;
255
256 for (pos_values = 0; pos_values < size && pos_values < MAX_VALUES_SIZE;
257 pos_values++) {
258 if (elem >= enum_obj_count) {
259 pr_err("Error enum-objects package is too small\n");
260 return -EINVAL;
261 }
262
263 ret = hp_convert_hexstr_to_str(enum_obj[elem + pos_values].string.pointer,
264 enum_obj[elem + pos_values].string.length,
265 &str_value, &value_len);
Re-assigned again.
266
267 if (ret)
268 return -EINVAL;
269
270 /*
271 * ignore strings when possible values size
272 * is greater than MAX_VALUES_SIZE
273 */
274 if (size < MAX_VALUES_SIZE)
275 strscpy(enum_data->possible_values[pos_values],
276 str_value,
277 sizeof(enum_data->possible_values[pos_values]));
278 }
279 break;
280 default:
281 pr_warn("Invalid element: %d found in Enumeration attribute or data may be malformed\n", elem);
282 break;
283 }
284
--> 285 kfree(str_value);
str_value is freed at the end of every iteration so this is double free
from the PREREQUISITES code.
286 }
287
288 exit_enumeration_package:
289 kfree(str_value);
This is a double free as well. I don't see how this one could have been
avoided in testing???
290 return 0;
291 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] platform/x86: hp-bioscfg: enum-attributes
2023-07-17 10:39 [bug report] platform/x86: hp-bioscfg: enum-attributes Dan Carpenter
@ 2023-07-18 10:55 ` Ilpo Järvinen
2023-07-25 19:49 ` Lopez, Jorge A (Security)
1 sibling, 0 replies; 3+ messages in thread
From: Ilpo Järvinen @ 2023-07-18 10:55 UTC (permalink / raw)
To: Dan Carpenter, jorge.lopez2; +Cc: platform-driver-x86
On Mon, 17 Jul 2023, Dan Carpenter wrote:
> Hello Jorge Lopez,
>
> The patch 6b2770bfd6f9: "platform/x86: hp-bioscfg: enum-attributes"
> from Jun 8, 2023 (linux-next), leads to the following Smatch static
> checker warning:
>
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c:285 hp_populate_enumeration_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c:289 hp_populate_enumeration_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/int-attributes.c:263 hp_populate_integer_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:286 hp_populate_ordered_list_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:290 hp_populate_ordered_list_elements_from_package() error: double free of 'tmpstr'
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:291 hp_populate_ordered_list_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c:371 hp_populate_password_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/string-attributes.c:252 hp_populate_string_elements_from_package() error: double free of 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/string-attributes.c:256 hp_populate_string_elements_from_package() error: double free of 'str_value'
>
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
> 125 static int hp_populate_enumeration_elements_from_package(union acpi_object *enum_obj,
> 126 int enum_obj_count,
> 127 int instance_id)
> 128 {
> 129 char *str_value = NULL;
>
> str_value starts as NULL.
>
> 130 int value_len;
> 131 u32 size = 0;
> 132 u32 int_value;
> 133 int elem = 0;
> 134 int reqs;
> 135 int pos_values;
> 136 int ret;
> 137 int eloc;
> 138 struct enumeration_data *enum_data = &bioscfg_drv.enumeration_data[instance_id];
> 139
> 140 for (elem = 1, eloc = 1; elem < enum_obj_count; elem++, eloc++) {
> 141 /* ONLY look at the first ENUM_ELEM_CNT elements */
> 142 if (eloc == ENUM_ELEM_CNT)
> 143 goto exit_enumeration_package;
>
> But here we free the str_value from the previous iteration.
>
> 144
> 145 switch (enum_obj[elem].type) {
> 146 case ACPI_TYPE_STRING:
> 147 if (PREREQUISITES != elem && ENUM_POSSIBLE_VALUES != elem) {
> 148 ret = hp_convert_hexstr_to_str(enum_obj[elem].string.pointer,
> 149 enum_obj[elem].string.length,
> 150 &str_value, &value_len);
> 151 if (ret)
> 152 return -EINVAL;
>
> Here the str_value from the previos iteration is re-assigned without
> being freed. (memory leak).
>
> 153 }
> 154 break;
> 155 case ACPI_TYPE_INTEGER:
> 156 int_value = (u32)enum_obj[elem].integer.value;
> 157 break;
> 158 default:
> 159 pr_warn("Unsupported object type [%d]\n", enum_obj[elem].type);
> 160 continue;
> 161 }
> 162
> 163 /* Check that both expected and read object type match */
> 164 if (expected_enum_types[eloc] != enum_obj[elem].type) {
> 165 pr_err("Error expected type %d for elem %d, but got type %d instead\n",
> 166 expected_enum_types[eloc], elem, enum_obj[elem].type);
> 167 return -EIO;
> 168 }
> 169
> 170 /* Assign appropriate element value to corresponding field */
> 171 switch (eloc) {
> 172 case NAME:
> 173 case VALUE:
> 174 break;
> 175 case PATH:
> 176 strscpy(enum_data->common.path, str_value,
>
> If str_value is NULL this will crash.
>
> 177 sizeof(enum_data->common.path));
> 178 break;
> 179 case IS_READONLY:
> 180 enum_data->common.is_readonly = int_value;
> 181 break;
> 182 case DISPLAY_IN_UI:
> 183 enum_data->common.display_in_ui = int_value;
> 184 break;
> 185 case REQUIRES_PHYSICAL_PRESENCE:
> 186 enum_data->common.requires_physical_presence = int_value;
> 187 break;
> 188 case SEQUENCE:
> 189 enum_data->common.sequence = int_value;
> 190 break;
> 191 case PREREQUISITES_SIZE:
> 192 enum_data->common.prerequisites_size = int_value;
> 193 if (int_value > MAX_PREREQUISITES_SIZE)
> 194 pr_warn("Prerequisites size value exceeded the maximum number of elements supported or data may be malformed\n");
> 195
> 196 /*
> 197 * This HACK is needed to keep the expected
> 198 * element list pointing to the right obj[elem].type
> 199 * when the size is zero. PREREQUISITES
> 200 * object is omitted by BIOS when the size is
> 201 * zero.
> 202 */
> 203 if (int_value == 0)
> 204 eloc++;
> 205 break;
> 206
> 207 case PREREQUISITES:
> 208 size = min_t(u32, enum_data->common.prerequisites_size, MAX_PREREQUISITES_SIZE);
> 209 for (reqs = 0; reqs < size; reqs++) {
> 210 if (elem >= enum_obj_count) {
> 211 pr_err("Error enum-objects package is too small\n");
> 212 return -EINVAL;
> 213 }
> 214
> 215 ret = hp_convert_hexstr_to_str(enum_obj[elem + reqs].string.pointer,
> 216 enum_obj[elem + reqs].string.length,
> 217 &str_value, &value_len);
>
> str_value is re-assigned again. (memory leak).
>
> 218
> 219 if (ret)
> 220 return -EINVAL;
> 221
> 222 strscpy(enum_data->common.prerequisites[reqs],
> 223 str_value,
> 224 sizeof(enum_data->common.prerequisites[reqs]));
> 225
> 226 kfree(str_value);
>
> str_value is freed. (this will lead to a crash).
>
> 227 }
> 228 break;
> 229
> 230 case SECURITY_LEVEL:
> 231 enum_data->common.security_level = int_value;
> 232 break;
> 233
> 234 case ENUM_CURRENT_VALUE:
> 235 strscpy(enum_data->current_value,
> 236 str_value, sizeof(enum_data->current_value));
>
> No check for NULL.
>
> 237 break;
> 238 case ENUM_SIZE:
> 239 enum_data->possible_values_size = int_value;
> 240 if (int_value > MAX_VALUES_SIZE)
> 241 pr_warn("Possible number values size value exceeded the maximum number of elements supported or data may be malformed\n");
> 242
> 243 /*
> 244 * This HACK is needed to keep the expected
> 245 * element list pointing to the right obj[elem].type
> 246 * when the size is zero. POSSIBLE_VALUES
> 247 * object is omitted by BIOS when the size is zero.
> 248 */
> 249 if (int_value == 0)
> 250 eloc++;
> 251 break;
> 252
> 253 case ENUM_POSSIBLE_VALUES:
> 254 size = enum_data->possible_values_size;
> 255
> 256 for (pos_values = 0; pos_values < size && pos_values < MAX_VALUES_SIZE;
> 257 pos_values++) {
> 258 if (elem >= enum_obj_count) {
> 259 pr_err("Error enum-objects package is too small\n");
> 260 return -EINVAL;
> 261 }
> 262
> 263 ret = hp_convert_hexstr_to_str(enum_obj[elem + pos_values].string.pointer,
> 264 enum_obj[elem + pos_values].string.length,
> 265 &str_value, &value_len);
>
> Re-assigned again.
>
> 266
> 267 if (ret)
> 268 return -EINVAL;
> 269
> 270 /*
> 271 * ignore strings when possible values size
> 272 * is greater than MAX_VALUES_SIZE
> 273 */
> 274 if (size < MAX_VALUES_SIZE)
> 275 strscpy(enum_data->possible_values[pos_values],
> 276 str_value,
> 277 sizeof(enum_data->possible_values[pos_values]));
> 278 }
> 279 break;
> 280 default:
> 281 pr_warn("Invalid element: %d found in Enumeration attribute or data may be malformed\n", elem);
> 282 break;
> 283 }
> 284
> --> 285 kfree(str_value);
>
> str_value is freed at the end of every iteration so this is double free
> from the PREREQUISITES code.
>
> 286 }
> 287
> 288 exit_enumeration_package:
> 289 kfree(str_value);
>
> This is a double free as well. I don't see how this one could have been
> avoided in testing???
>
> 290 return 0;
> 291 }
I found what looked like even triple frees during my review of this series
against one of these constructs [1]. The whole series was full of
copy-pasted code so the same bugs and problems probably repeat over and over.
Due to copy-pasted code, I suggested moving common code into helpers which
would have simplified these functions significantly but like you see, not
much really happened (and seemingly not even the bugs that I explicitly
mentioned were addressed :-().
Diffing some of these functions yields:
...
case PATH:
- strscpy(enum_data->common.path, str_value,
- sizeof(enum_data->common.path));
+ strscpy(ordered_list_data->common.path, str_value,
+ sizeof(ordered_list_data->common.path));
break;
case IS_READONLY:
- enum_data->common.is_readonly = int_value;
+ ordered_list_data->common.is_readonly = int_value;
break;
case DISPLAY_IN_UI:
- enum_data->common.display_in_ui = int_value;
+ ordered_list_data->common.display_in_ui = int_value;
break;
case REQUIRES_PHYSICAL_PRESENCE:
- enum_data->common.requires_physical_presence = int_value;
+ ordered_list_data->common.requires_physical_presence = int_value;
break;
case SEQUENCE:
- enum_data->common.sequence = int_value;
+ ordered_list_data->common.sequence = int_value;
break;
case PREREQUISITES_SIZE:
- enum_data->common.prerequisites_size = int_value;
+ ordered_list_data->common.prerequisites_size = int_value;
if (int_value > MAX_PREREQUISITES_SIZE)
pr_warn("Prerequisites size value exceeded the maximum number of e>
...
...that part of the struct is even called "common" so it should be pretty
obvious there might be some common code.
[1] https://patchwork.kernel.org/project/platform-driver-x86/patch/20230505220043.39036-6-jorge.lopez2@hp.com/#25328544
--
i.
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [bug report] platform/x86: hp-bioscfg: enum-attributes
2023-07-17 10:39 [bug report] platform/x86: hp-bioscfg: enum-attributes Dan Carpenter
2023-07-18 10:55 ` Ilpo Järvinen
@ 2023-07-25 19:49 ` Lopez, Jorge A (Security)
1 sibling, 0 replies; 3+ messages in thread
From: Lopez, Jorge A (Security) @ 2023-07-25 19:49 UTC (permalink / raw)
To: Dan Carpenter; +Cc: platform-driver-x86@vger.kernel.org
Hi Dan,
I will submit a patch to address memory leaks in hp_populate_enumeration_elements_from_package() reported here and to address some uninitialized variable errors reported in a separate email.
Regards,
Jorge Lopez
HP Inc
"Once you stop learning, you start dying"
Albert Einstein
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@linaro.org>
> Sent: Monday, July 17, 2023 5:40 AM
> To: Lopez, Jorge A (Security) <jorge.lopez2@hp.com>
> Cc: platform-driver-x86@vger.kernel.org
> Subject: [bug report] platform/x86: hp-bioscfg: enum-attributes
>
> CAUTION: External Email
>
> Hello Jorge Lopez,
>
> The patch 6b2770bfd6f9: "platform/x86: hp-bioscfg: enum-attributes"
> from Jun 8, 2023 (linux-next), leads to the following Smatch static checker
> warning:
>
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c:285
> hp_populate_enumeration_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c:289
> hp_populate_enumeration_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/int-attributes.c:263
> hp_populate_integer_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:286
> hp_populate_ordered_list_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:290
> hp_populate_ordered_list_elements_from_package() error: double free of
> 'tmpstr'
> drivers/platform/x86/hp/hp-bioscfg/order-list-attributes.c:291
> hp_populate_ordered_list_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c:371
> hp_populate_password_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/string-attributes.c:252
> hp_populate_string_elements_from_package() error: double free of
> 'str_value'
> drivers/platform/x86/hp/hp-bioscfg/string-attributes.c:256
> hp_populate_string_elements_from_package() error: double free of
> 'str_value'
>
> drivers/platform/x86/hp/hp-bioscfg/enum-attributes.c
> 125 static int hp_populate_enumeration_elements_from_package(union
> acpi_object *enum_obj,
> 126 int enum_obj_count,
> 127 int instance_id)
> 128 {
> 129 char *str_value = NULL;
>
> str_value starts as NULL.
>
> 130 int value_len;
> 131 u32 size = 0;
> 132 u32 int_value;
> 133 int elem = 0;
> 134 int reqs;
> 135 int pos_values;
> 136 int ret;
> 137 int eloc;
> 138 struct enumeration_data *enum_data =
> &bioscfg_drv.enumeration_data[instance_id];
> 139
> 140 for (elem = 1, eloc = 1; elem < enum_obj_count; elem++, eloc++) {
> 141 /* ONLY look at the first ENUM_ELEM_CNT elements */
> 142 if (eloc == ENUM_ELEM_CNT)
> 143 goto exit_enumeration_package;
>
> But here we free the str_value from the previous iteration.
>
> 144
> 145 switch (enum_obj[elem].type) {
> 146 case ACPI_TYPE_STRING:
> 147 if (PREREQUISITES != elem && ENUM_POSSIBLE_VALUES
> != elem) {
> 148 ret =
> hp_convert_hexstr_to_str(enum_obj[elem].string.pointer,
> 149 enum_obj[elem].string.length,
> 150 &str_value, &value_len);
> 151 if (ret)
> 152 return -EINVAL;
>
> Here the str_value from the previos iteration is re-assigned without being
> freed. (memory leak).
>
> 153 }
> 154 break;
> 155 case ACPI_TYPE_INTEGER:
> 156 int_value = (u32)enum_obj[elem].integer.value;
> 157 break;
> 158 default:
> 159 pr_warn("Unsupported object type [%d]\n",
> enum_obj[elem].type);
> 160 continue;
> 161 }
> 162
> 163 /* Check that both expected and read object type match */
> 164 if (expected_enum_types[eloc] != enum_obj[elem].type) {
> 165 pr_err("Error expected type %d for elem %d, but got
> type %d instead\n",
> 166 expected_enum_types[eloc], elem,
> enum_obj[elem].type);
> 167 return -EIO;
> 168 }
> 169
> 170 /* Assign appropriate element value to corresponding field
> */
> 171 switch (eloc) {
> 172 case NAME:
> 173 case VALUE:
> 174 break;
> 175 case PATH:
> 176 strscpy(enum_data->common.path, str_value,
>
> If str_value is NULL this will crash.
>
> 177 sizeof(enum_data->common.path));
> 178 break;
> 179 case IS_READONLY:
> 180 enum_data->common.is_readonly = int_value;
> 181 break;
> 182 case DISPLAY_IN_UI:
> 183 enum_data->common.display_in_ui = int_value;
> 184 break;
> 185 case REQUIRES_PHYSICAL_PRESENCE:
> 186 enum_data->common.requires_physical_presence =
> int_value;
> 187 break;
> 188 case SEQUENCE:
> 189 enum_data->common.sequence = int_value;
> 190 break;
> 191 case PREREQUISITES_SIZE:
> 192 enum_data->common.prerequisites_size = int_value;
> 193 if (int_value > MAX_PREREQUISITES_SIZE)
> 194 pr_warn("Prerequisites size value exceeded the
> maximum number of elements supported or data may be malformed\n");
> 195
> 196 /*
> 197 * This HACK is needed to keep the expected
> 198 * element list pointing to the right obj[elem].type
> 199 * when the size is zero. PREREQUISITES
> 200 * object is omitted by BIOS when the size is
> 201 * zero.
> 202 */
> 203 if (int_value == 0)
> 204 eloc++;
> 205 break;
> 206
> 207 case PREREQUISITES:
> 208 size = min_t(u32, enum_data-
> >common.prerequisites_size, MAX_PREREQUISITES_SIZE);
> 209 for (reqs = 0; reqs < size; reqs++) {
> 210 if (elem >= enum_obj_count) {
> 211 pr_err("Error enum-objects package is too
> small\n");
> 212 return -EINVAL;
> 213 }
> 214
> 215 ret = hp_convert_hexstr_to_str(enum_obj[elem +
> reqs].string.pointer,
> 216 enum_obj[elem +
> reqs].string.length,
> 217 &str_value, &value_len);
>
> str_value is re-assigned again. (memory leak).
>
> 218
> 219 if (ret)
> 220 return -EINVAL;
> 221
> 222 strscpy(enum_data->common.prerequisites[reqs],
> 223 str_value,
> 224 sizeof(enum_data-
> >common.prerequisites[reqs]));
> 225
> 226 kfree(str_value);
>
> str_value is freed. (this will lead to a crash).
>
> 227 }
> 228 break;
> 229
> 230 case SECURITY_LEVEL:
> 231 enum_data->common.security_level = int_value;
> 232 break;
> 233
> 234 case ENUM_CURRENT_VALUE:
> 235 strscpy(enum_data->current_value,
> 236 str_value, sizeof(enum_data->current_value));
>
> No check for NULL.
>
> 237 break;
> 238 case ENUM_SIZE:
> 239 enum_data->possible_values_size = int_value;
> 240 if (int_value > MAX_VALUES_SIZE)
> 241 pr_warn("Possible number values size value
> exceeded the maximum number of elements supported or data may be
> malformed\n");
> 242
> 243 /*
> 244 * This HACK is needed to keep the expected
> 245 * element list pointing to the right obj[elem].type
> 246 * when the size is zero. POSSIBLE_VALUES
> 247 * object is omitted by BIOS when the size is zero.
> 248 */
> 249 if (int_value == 0)
> 250 eloc++;
> 251 break;
> 252
> 253 case ENUM_POSSIBLE_VALUES:
> 254 size = enum_data->possible_values_size;
> 255
> 256 for (pos_values = 0; pos_values < size && pos_values <
> MAX_VALUES_SIZE;
> 257 pos_values++) {
> 258 if (elem >= enum_obj_count) {
> 259 pr_err("Error enum-objects package is too
> small\n");
> 260 return -EINVAL;
> 261 }
> 262
> 263 ret = hp_convert_hexstr_to_str(enum_obj[elem +
> pos_values].string.pointer,
> 264 enum_obj[elem +
> pos_values].string.length,
> 265 &str_value, &value_len);
>
> Re-assigned again.
>
> 266
> 267 if (ret)
> 268 return -EINVAL;
> 269
> 270 /*
> 271 * ignore strings when possible values size
> 272 * is greater than MAX_VALUES_SIZE
> 273 */
> 274 if (size < MAX_VALUES_SIZE)
> 275 strscpy(enum_data-
> >possible_values[pos_values],
> 276 str_value,
> 277 sizeof(enum_data-
> >possible_values[pos_values]));
> 278 }
> 279 break;
> 280 default:
> 281 pr_warn("Invalid element: %d found in Enumeration
> attribute or data may be malformed\n", elem);
> 282 break;
> 283 }
> 284
> --> 285 kfree(str_value);
>
> str_value is freed at the end of every iteration so this is double free from
> the PREREQUISITES code.
>
> 286 }
> 287
> 288 exit_enumeration_package:
> 289 kfree(str_value);
>
> This is a double free as well. I don't see how this one could have been
> avoided in testing???
>
> 290 return 0;
> 291 }
>
> regards,
> dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-25 19:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-07-17 10:39 [bug report] platform/x86: hp-bioscfg: enum-attributes Dan Carpenter
2023-07-18 10:55 ` Ilpo Järvinen
2023-07-25 19:49 ` Lopez, Jorge A (Security)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox