* [bug report] tracing, synthetic events: Replace buggy strcat() with seq_buf operations
@ 2020-11-02 7:45 Dan Carpenter
2020-11-02 16:39 ` Steven Rostedt
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2020-11-02 7:45 UTC (permalink / raw)
To: kernel-janitors
Hello Steven Rostedt (VMware),
The patch 761a8c58db6b: "tracing, synthetic events: Replace buggy
strcat() with seq_buf operations" from Oct 23, 2020, leads to the
following static checker warning:
kernel/trace/trace_events_synth.c:703 parse_synth_field()
warn: passing zero to 'ERR_PTR'
kernel/trace/trace_events_synth.c
582 static struct synth_field *parse_synth_field(int argc, const char **argv,
583 int *consumed)
584 {
585 struct synth_field *field;
586 const char *prefix = NULL, *field_type = argv[0], *field_name, *array;
587 int len, ret = 0;
588 struct seq_buf s;
589 ssize_t size;
590
591 if (field_type[0] = ';')
592 field_type++;
593
594 if (!strcmp(field_type, "unsigned")) {
595 if (argc < 3) {
596 synth_err(SYNTH_ERR_INCOMPLETE_TYPE, errpos(field_type));
597 return ERR_PTR(-EINVAL);
598 }
599 prefix = "unsigned ";
600 field_type = argv[1];
601 field_name = argv[2];
602 *consumed = 3;
603 } else {
604 field_name = argv[1];
605 *consumed = 2;
606 }
607
608 field = kzalloc(sizeof(*field), GFP_KERNEL);
609 if (!field)
610 return ERR_PTR(-ENOMEM);
611
612 len = strlen(field_name);
613 array = strchr(field_name, '[');
614 if (array)
615 len -= strlen(array);
616 else if (field_name[len - 1] = ';')
617 len--;
618
619 field->name = kmemdup_nul(field_name, len, GFP_KERNEL);
620 if (!field->name) {
621 ret = -ENOMEM;
622 goto free;
623 }
624 if (!is_good_name(field->name)) {
625 synth_err(SYNTH_ERR_BAD_NAME, errpos(field_name));
626 ret = -EINVAL;
627 goto free;
628 }
629
630 if (field_type[0] = ';')
631 field_type++;
632 len = strlen(field_type) + 1;
633
634 if (array)
635 len += strlen(array);
636
637 if (prefix)
638 len += strlen(prefix);
639
640 field->type = kzalloc(len, GFP_KERNEL);
641 if (!field->type) {
642 ret = -ENOMEM;
643 goto free;
644 }
645 seq_buf_init(&s, field->type, len);
646 if (prefix)
647 seq_buf_puts(&s, prefix);
648 seq_buf_puts(&s, field_type);
649 if (array) {
650 seq_buf_puts(&s, array);
651 if (s.buffer[s.len - 1] = ';')
652 s.len--;
653 }
654 if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
655 goto free;
This was originally reported by kbuild-bot, but it was somehow
overlooked so now it's on my system. The missing error code will lead
to a NULL dereference in the caller.
656 s.buffer[s.len] = '\0';
657
658 size = synth_field_size(field->type);
659 if (size < 0) {
660 synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
661 ret = -EINVAL;
662 goto free;
663 } else if (size = 0) {
664 if (synth_field_is_string(field->type)) {
665 char *type;
666
667 len = sizeof("__data_loc ") + strlen(field->type) + 1;
668 type = kzalloc(len, GFP_KERNEL);
669 if (!type) {
670 ret = -ENOMEM;
671 goto free;
672 }
673
674 seq_buf_init(&s, type, len);
675 seq_buf_puts(&s, "__data_loc ");
676 seq_buf_puts(&s, field->type);
677
678 if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
679 goto free;
Here as well.
680 s.buffer[s.len] = '\0';
681
682 kfree(field->type);
683 field->type = type;
684
685 field->is_dynamic = true;
686 size = sizeof(u64);
687 } else {
688 synth_err(SYNTH_ERR_INVALID_TYPE, errpos(field_type));
689 ret = -EINVAL;
690 goto free;
691 }
692 }
693 field->size = size;
694
695 if (synth_field_is_string(field->type))
696 field->is_string = true;
697
698 field->is_signed = synth_field_signed(field->type);
699 out:
700 return field;
701 free:
702 free_synth_field(field);
703 field = ERR_PTR(ret);
704 goto out;
705 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread* Re: [bug report] tracing, synthetic events: Replace buggy strcat() with seq_buf operations 2020-11-02 7:45 [bug report] tracing, synthetic events: Replace buggy strcat() with seq_buf operations Dan Carpenter @ 2020-11-02 16:39 ` Steven Rostedt 0 siblings, 0 replies; 2+ messages in thread From: Steven Rostedt @ 2020-11-02 16:39 UTC (permalink / raw) To: Dan Carpenter; +Cc: kernel-janitors, Tom Zanussi, LKML On Mon, 2 Nov 2020 10:45:24 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > Hello Steven Rostedt (VMware), > > The patch 761a8c58db6b: "tracing, synthetic events: Replace buggy > strcat() with seq_buf operations" from Oct 23, 2020, leads to the > following static checker warning: > > kernel/trace/trace_events_synth.c:703 parse_synth_field() > warn: passing zero to 'ERR_PTR' > > kernel/trace/trace_events_synth.c > 582 static struct synth_field *parse_synth_field(int argc, const char **argv, > 583 int *consumed) > 584 { > 585 struct synth_field *field; > 586 const char *prefix = NULL, *field_type = argv[0], *field_name, *array; > 587 int len, ret = 0; > 588 struct seq_buf s; > 589 ssize_t size; > 590 > 591 if (field_type[0] = ';') > 592 field_type++; > 593 > 594 if (!strcmp(field_type, "unsigned")) { > 595 if (argc < 3) { > 596 synth_err(SYNTH_ERR_INCOMPLETE_TYPE, errpos(field_type)); > 597 return ERR_PTR(-EINVAL); > 598 } > 599 prefix = "unsigned "; > 600 field_type = argv[1]; > 601 field_name = argv[2]; > 602 *consumed = 3; > 603 } else { > 604 field_name = argv[1]; > 605 *consumed = 2; > 606 } > 607 > 608 field = kzalloc(sizeof(*field), GFP_KERNEL); > 609 if (!field) > 610 return ERR_PTR(-ENOMEM); > 611 > 612 len = strlen(field_name); > 613 array = strchr(field_name, '['); > 614 if (array) > 615 len -= strlen(array); > 616 else if (field_name[len - 1] = ';') > 617 len--; > 618 > 619 field->name = kmemdup_nul(field_name, len, GFP_KERNEL); > 620 if (!field->name) { > 621 ret = -ENOMEM; > 622 goto free; > 623 } > 624 if (!is_good_name(field->name)) { > 625 synth_err(SYNTH_ERR_BAD_NAME, errpos(field_name)); > 626 ret = -EINVAL; > 627 goto free; > 628 } > 629 > 630 if (field_type[0] = ';') > 631 field_type++; > 632 len = strlen(field_type) + 1; > 633 > 634 if (array) > 635 len += strlen(array); > 636 > 637 if (prefix) > 638 len += strlen(prefix); > 639 > 640 field->type = kzalloc(len, GFP_KERNEL); > 641 if (!field->type) { > 642 ret = -ENOMEM; > 643 goto free; > 644 } > 645 seq_buf_init(&s, field->type, len); > 646 if (prefix) > 647 seq_buf_puts(&s, prefix); > 648 seq_buf_puts(&s, field_type); > 649 if (array) { > 650 seq_buf_puts(&s, array); > 651 if (s.buffer[s.len - 1] = ';') > 652 s.len--; > 653 } > 654 if (WARN_ON_ONCE(!seq_buf_buffer_left(&s))) > 655 goto free; > > This was originally reported by kbuild-bot, but it was somehow > overlooked so now it's on my system. The missing error code will lead > to a NULL dereference in the caller. I misread the original report, modified it to fix something else, and didn't see the real problem. Having to initialize ret for *every* error path is ridiculous. Here's the fix. -- Steve From 2980f226a7fb08e37fd3948e206854fe5a1a8c50 Mon Sep 17 00:00:00 2001 From: "Steven Rostedt (VMware)" <rostedt@goodmis.org> Date: Mon, 2 Nov 2020 11:28:39 -0500 Subject: [PATCH] tracing: Make -ENOMEM the default error for parse_synth_field() parse_synth_field() returns a pointer and requires that errors get surrounded by ERR_PTR(). The ret variable is initialized to zero, but should never be used as zero, and if it is, it could cause a false return code and produce a NULL pointer dereference. It makes no sense to set ret to zero. Set ret to -ENOMEM (the most common error case), and have any other errors set it to something else. This removes the need to initialize ret on *every* error branch. Fixes: 761a8c58db6b ("tracing, synthetic events: Replace buggy strcat() with seq_buf operations") Reported-by: Dan Carpenter <dan.carpenter@oracle.com> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org> --- kernel/trace/trace_events_synth.c | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/kernel/trace/trace_events_synth.c b/kernel/trace/trace_events_synth.c index 84b7cab55291..881df991742a 100644 --- a/kernel/trace/trace_events_synth.c +++ b/kernel/trace/trace_events_synth.c @@ -584,7 +584,7 @@ static struct synth_field *parse_synth_field(int argc, const char **argv, { struct synth_field *field; const char *prefix = NULL, *field_type = argv[0], *field_name, *array; - int len, ret = 0; + int len, ret = -ENOMEM; struct seq_buf s; ssize_t size; @@ -617,10 +617,9 @@ static struct synth_field *parse_synth_field(int argc, const char **argv, len--; field->name = kmemdup_nul(field_name, len, GFP_KERNEL); - if (!field->name) { - ret = -ENOMEM; + if (!field->name) goto free; - } + if (!is_good_name(field->name)) { synth_err(SYNTH_ERR_BAD_NAME, errpos(field_name)); ret = -EINVAL; @@ -638,10 +637,9 @@ static struct synth_field *parse_synth_field(int argc, const char **argv, len += strlen(prefix); field->type = kzalloc(len, GFP_KERNEL); - if (!field->type) { - ret = -ENOMEM; + if (!field->type) goto free; - } + seq_buf_init(&s, field->type, len); if (prefix) seq_buf_puts(&s, prefix); @@ -653,6 +651,7 @@ static struct synth_field *parse_synth_field(int argc, const char **argv, } if (WARN_ON_ONCE(!seq_buf_buffer_left(&s))) goto free; + s.buffer[s.len] = '\0'; size = synth_field_size(field->type); @@ -666,10 +665,8 @@ static struct synth_field *parse_synth_field(int argc, const char **argv, len = sizeof("__data_loc ") + strlen(field->type) + 1; type = kzalloc(len, GFP_KERNEL); - if (!type) { - ret = -ENOMEM; + if (!type) goto free; - } seq_buf_init(&s, type, len); seq_buf_puts(&s, "__data_loc "); -- 2.25.4 ^ permalink raw reply related [flat|nested] 2+ messages in thread
end of thread, other threads:[~2020-11-02 16:39 UTC | newest] Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-02 7:45 [bug report] tracing, synthetic events: Replace buggy strcat() with seq_buf operations Dan Carpenter 2020-11-02 16:39 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).