Hi Steve,
On Mon, 2020-10-26 at 12:53 -0400, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rost...@goodmis.org>
>
> There was a memory corruption bug happening while running the
> synthetic
> event selftests:
>
> kmemleak: Cannot insert 0xffff8c196fa2afe5 into the object search
> tree (overlaps existing)
> CPU: 5 PID: 6866 Comm: ftracetest Tainted: G W 5.9.0-
> rc5-test+ #577
> Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01
> v03.03 07/14/2016
> Call Trace:
> dump_stack+0x8d/0xc0
> create_object.cold+0x3b/0x60
> slab_post_alloc_hook+0x57/0x510
> ? tracing_map_init+0x178/0x340
> __kmalloc+0x1b1/0x390
> tracing_map_init+0x178/0x340
> event_hist_trigger_func+0x523/0xa40
> trigger_process_regex+0xc5/0x110
> event_trigger_write+0x71/0xd0
> vfs_write+0xca/0x210
> ksys_write+0x70/0xf0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
> RIP: 0033:0x7fef0a63a487
> Code: 64 89 02 48 c7 c0 ff ff ff ff eb bb 0f 1f 80 00 00 00 00 f3 0f
> 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 b8 01 00 00 00 0f 05 <48>
> 3d 00 f0 ff ff 77 51 c3 48 83 ec 28 48 89 54 24 18 48 89 74 24
> RSP: 002b:00007fff76f18398 EFLAGS: 00000246 ORIG_RAX:
> 0000000000000001
> RAX: ffffffffffffffda RBX: 0000000000000039 RCX: 00007fef0a63a487
> RDX: 0000000000000039 RSI: 000055eb3b26d690 RDI: 0000000000000001
> RBP: 000055eb3b26d690 R08: 000000000000000a R09: 0000000000000038
> R10: 000055eb3b2cdb80 R11: 0000000000000246 R12: 0000000000000039
> R13: 00007fef0a70b500 R14: 0000000000000039 R15: 00007fef0a70b700
> kmemleak: Kernel memory leak detector disabled
> kmemleak: Object 0xffff8c196fa2afe0 (size 8):
> kmemleak: comm "ftracetest", pid 6866, jiffies 4295082531
> kmemleak: min_count = 1
> kmemleak: count = 0
> kmemleak: flags = 0x1
> kmemleak: checksum = 0
> kmemleak: backtrace:
> __kmalloc+0x1b1/0x390
> tracing_map_init+0x1be/0x340
> event_hist_trigger_func+0x523/0xa40
> trigger_process_regex+0xc5/0x110
> event_trigger_write+0x71/0xd0
> vfs_write+0xca/0x210
> ksys_write+0x70/0xf0
> do_syscall_64+0x33/0x40
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> The cause came down to a use of strcat() that was adding an string
> that was
> shorten, but the strcat() did not take that into account.
>
> strcat() is extremely dangerous as it does not care how big the
> buffer is.
> Replace it with seq_buf operations that prevent the buffer from being
> overwritten if what is being written is bigger than the buffer.
>
> Fixes: 10819e25799a ("tracing: Handle synthetic event array field
> type checking correctly")
> Signed-off-by: Steven Rostedt (VMware) <rost...@goodmis.org>
> ---
>
> Changes since v1, Dan's scripts detected a double free.
> Just needed to move, the freeing after the error branch
> to freeing.
>
Looks fine to me, thanks for fixing this.
Reviewed-by: Tom Zanussi <zanu...@kernel.org>
Tested-by: Tom Zanussi <zanu...@kernel.org>
> kernel/trace/trace_events_synth.c | 36 +++++++++++++++++++--------
> ----
> 1 file changed, 22 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/trace/trace_events_synth.c
> b/kernel/trace/trace_events_synth.c
> index 3212e2c653b3..84b7cab55291 100644
> --- a/kernel/trace/trace_events_synth.c
> +++ b/kernel/trace/trace_events_synth.c
> @@ -585,6 +585,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;
> + struct seq_buf s;
> ssize_t size;
>
> if (field_type[0] == ';')
> @@ -630,13 +631,9 @@ static struct synth_field *parse_synth_field(int
> argc, const char **argv,
> field_type++;
> len = strlen(field_type) + 1;
>
> - if (array) {
> - int l = strlen(array);
> + if (array)
> + len += strlen(array);
>
> - if (l && array[l - 1] == ';')
> - l--;
> - len += l;
> - }
> if (prefix)
> len += strlen(prefix);
>
> @@ -645,14 +642,18 @@ static struct synth_field
> *parse_synth_field(int argc, const char **argv,
> ret = -ENOMEM;
> goto free;
> }
> + seq_buf_init(&s, field->type, len);
> if (prefix)
> - strcat(field->type, prefix);
> - strcat(field->type, field_type);
> + seq_buf_puts(&s, prefix);
> + seq_buf_puts(&s, field_type);
> if (array) {
> - strcat(field->type, array);
> - if (field->type[len - 1] == ';')
> - field->type[len - 1] = '\0';
> + seq_buf_puts(&s, array);
> + if (s.buffer[s.len - 1] == ';')
> + s.len--;
> }
> + if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
> + goto free;
> + s.buffer[s.len] = '\0';
>
> size = synth_field_size(field->type);
> if (size < 0) {
> @@ -663,14 +664,21 @@ static struct synth_field
> *parse_synth_field(int argc, const char **argv,
> if (synth_field_is_string(field->type)) {
> char *type;
>
> - type = kzalloc(sizeof("__data_loc ") +
> strlen(field->type) + 1, GFP_KERNEL);
> + len = sizeof("__data_loc ") + strlen(field-
> >type) + 1;
> + type = kzalloc(len, GFP_KERNEL);
> if (!type) {
> ret = -ENOMEM;
> goto free;
> }
>
> - strcat(type, "__data_loc ");
> - strcat(type, field->type);
> + seq_buf_init(&s, type, len);
> + seq_buf_puts(&s, "__data_loc ");
> + seq_buf_puts(&s, field->type);
> +
> + if (WARN_ON_ONCE(!seq_buf_buffer_left(&s)))
> + goto free;
> + s.buffer[s.len] = '\0';
> +
> kfree(field->type);
> field->type = type;
>
