* [bug report] user_events: Add minimal support for trace_event into ftrace
@ 2022-02-24 10:53 Dan Carpenter
2022-02-24 16:54 ` Beau Belgrave
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2022-02-24 10:53 UTC (permalink / raw)
To: beaub; +Cc: bpf
Hello Beau Belgrave,
The patch 7f5a08c79df3: "user_events: Add minimal support for
trace_event into ftrace" from Jan 18, 2022, leads to the following
Smatch static checker warning:
kernel/trace/trace_events_user.c:399 user_event_parse_field()
error: uninitialized symbol 'name'.
kernel/trace/trace_events_user.c
314 static int user_event_parse_field(char *field, struct user_event *user,
315 u32 *offset)
316 {
317 char *part, *type, *name;
318 u32 depth = 0, saved_offset = *offset;
319 int len, size = -EINVAL;
320 bool is_struct = false;
321
322 field = skip_spaces(field);
323
324 if (*field == '\0')
325 return 0;
326
327 /* Handle types that have a space within */
328 len = str_has_prefix(field, "unsigned ");
329 if (len)
330 goto skip_next;
331
332 len = str_has_prefix(field, "struct ");
333 if (len) {
334 is_struct = true;
335 goto skip_next;
336 }
337
338 len = str_has_prefix(field, "__data_loc unsigned ");
339 if (len)
340 goto skip_next;
341
342 len = str_has_prefix(field, "__data_loc ");
343 if (len)
344 goto skip_next;
345
346 len = str_has_prefix(field, "__rel_loc unsigned ");
347 if (len)
348 goto skip_next;
349
350 len = str_has_prefix(field, "__rel_loc ");
351 if (len)
352 goto skip_next;
353
354 goto parse;
355 skip_next:
356 type = field;
357 field = strpbrk(field + len, " ");
358
359 if (field == NULL)
360 return -EINVAL;
361
362 *field++ = '\0';
363 depth++;
364 parse:
365 while ((part = strsep(&field, " ")) != NULL) {
366 switch (depth++) {
367 case FIELD_DEPTH_TYPE:
368 type = part;
369 break;
370 case FIELD_DEPTH_NAME:
371 name = part;
^^^^^^^^^^^
name is only initialized here. Otherwise uninitialized.
372 break;
373 case FIELD_DEPTH_SIZE:
374 if (!is_struct)
375 return -EINVAL;
376
377 if (kstrtou32(part, 10, &size))
378 return -EINVAL;
379 break;
380 default:
381 return -EINVAL;
382 }
383 }
384
385 if (depth < FIELD_DEPTH_SIZE)
386 return -EINVAL;
387
388 if (depth == FIELD_DEPTH_SIZE)
389 size = user_field_size(type);
390
391 if (size == 0)
392 return -EINVAL;
393
394 if (size < 0)
395 return size;
396
397 *offset = saved_offset + size;
398
--> 399 return user_event_add_field(user, type, name, saved_offset, size,
400 type[0] != 'u', FILTER_OTHER);
401 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] user_events: Add minimal support for trace_event into ftrace
2022-02-24 10:53 [bug report] user_events: Add minimal support for trace_event into ftrace Dan Carpenter
@ 2022-02-24 16:54 ` Beau Belgrave
0 siblings, 0 replies; 2+ messages in thread
From: Beau Belgrave @ 2022-02-24 16:54 UTC (permalink / raw)
To: Dan Carpenter; +Cc: bpf
On Thu, Feb 24, 2022 at 01:53:34PM +0300, Dan Carpenter wrote:
> Hello Beau Belgrave,
>
> The patch 7f5a08c79df3: "user_events: Add minimal support for
> trace_event into ftrace" from Jan 18, 2022, leads to the following
> Smatch static checker warning:
>
> kernel/trace/trace_events_user.c:399 user_event_parse_field()
> error: uninitialized symbol 'name'.
>
> kernel/trace/trace_events_user.c
> 314 static int user_event_parse_field(char *field, struct user_event *user,
> 315 u32 *offset)
> 316 {
> 317 char *part, *type, *name;
> 318 u32 depth = 0, saved_offset = *offset;
> 319 int len, size = -EINVAL;
> 320 bool is_struct = false;
> 321
> 322 field = skip_spaces(field);
> 323
> 324 if (*field == '\0')
> 325 return 0;
> 326
> 327 /* Handle types that have a space within */
> 328 len = str_has_prefix(field, "unsigned ");
> 329 if (len)
> 330 goto skip_next;
> 331
> 332 len = str_has_prefix(field, "struct ");
> 333 if (len) {
> 334 is_struct = true;
> 335 goto skip_next;
> 336 }
> 337
> 338 len = str_has_prefix(field, "__data_loc unsigned ");
> 339 if (len)
> 340 goto skip_next;
> 341
> 342 len = str_has_prefix(field, "__data_loc ");
> 343 if (len)
> 344 goto skip_next;
> 345
> 346 len = str_has_prefix(field, "__rel_loc unsigned ");
> 347 if (len)
> 348 goto skip_next;
> 349
> 350 len = str_has_prefix(field, "__rel_loc ");
> 351 if (len)
> 352 goto skip_next;
> 353
> 354 goto parse;
> 355 skip_next:
> 356 type = field;
> 357 field = strpbrk(field + len, " ");
> 358
> 359 if (field == NULL)
> 360 return -EINVAL;
> 361
> 362 *field++ = '\0';
> 363 depth++;
> 364 parse:
> 365 while ((part = strsep(&field, " ")) != NULL) {
> 366 switch (depth++) {
> 367 case FIELD_DEPTH_TYPE:
> 368 type = part;
> 369 break;
> 370 case FIELD_DEPTH_NAME:
> 371 name = part;
> ^^^^^^^^^^^
> name is only initialized here. Otherwise uninitialized.
>
> 372 break;
> 373 case FIELD_DEPTH_SIZE:
> 374 if (!is_struct)
> 375 return -EINVAL;
> 376
> 377 if (kstrtou32(part, 10, &size))
> 378 return -EINVAL;
> 379 break;
> 380 default:
> 381 return -EINVAL;
> 382 }
> 383 }
> 384
> 385 if (depth < FIELD_DEPTH_SIZE)
> 386 return -EINVAL;
> 387
> 388 if (depth == FIELD_DEPTH_SIZE)
> 389 size = user_field_size(type);
> 390
> 391 if (size == 0)
> 392 return -EINVAL;
> 393
> 394 if (size < 0)
> 395 return size;
> 396
> 397 *offset = saved_offset + size;
> 398
> --> 399 return user_event_add_field(user, type, name, saved_offset, size,
> 400 type[0] != 'u', FILTER_OTHER);
> 401 }
>
> regards,
> dan carpenter
If name was not set the depth would be less than FIELD_DEPTH_SIZE.
Line 385 should protect against this.
Do you have a repro string that you believe would trigger this?
I can further protect against this by simply setting name to NULL at the
start and adding a check if you believe the case is valid.
Thanks,
-Beau
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-02-24 16:54 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-02-24 10:53 [bug report] user_events: Add minimal support for trace_event into ftrace Dan Carpenter
2022-02-24 16:54 ` Beau Belgrave
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox