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
