| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <77867e93-7785-457c-9e37-4b41503bb509@moroto.mountain>
Date: Thu, 27 Jul 2023 09:21:04 +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
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 }
Powered by blists - more mailing lists