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; >