On Mon, 2 Nov 2020 10:45:24 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxxx>
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@xxxxxxxxxx>
Signed-off-by: Steven Rostedt (VMware) <rostedt@xxxxxxxxxxx>
---
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
