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
