* [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).