From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.6 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B0A98C4363A for ; Mon, 26 Oct 2020 18:50:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 53D8321D7B for ; Mon, 26 Oct 2020 18:50:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603738200; bh=55QRZhxIDeLuMT0qzLtsHRGbu+SpFgd+RBNCY3NlGXc=; h=Subject:From:To:Cc:Date:In-Reply-To:References:List-ID:From; b=o7qnI+SHKblyI0II5vOfkzJjTWs4XyIVaTshjsrW+uPxcAJfopQmjc0CYgnesmCJm VdSX40B+WuIZpTIxbI0gDkMzCJHdyCvf9lFvqV1korfmHyXOalTi5D2tQRpOpe2XWI DzHv4mUZRg53QW6V/vbu5xc7TIHa7UemVGuvpilQ= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1790426AbgJZSt6 (ORCPT ); Mon, 26 Oct 2020 14:49:58 -0400 Received: from mail.kernel.org ([198.145.29.99]:42052 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1790418AbgJZSt6 (ORCPT ); Mon, 26 Oct 2020 14:49:58 -0400 Received: from tzanussi-mobl (c-73-209-127-30.hsd1.il.comcast.net [73.209.127.30]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 3A1CC2085B; Mon, 26 Oct 2020 18:49:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1603738197; bh=55QRZhxIDeLuMT0qzLtsHRGbu+SpFgd+RBNCY3NlGXc=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=vCacWCS8r+9eE7BExNc/r6W5ENROZIn51mjHty8CLLmXSI/DVC9bv38b13Ze38FiV QVW+zvai4+BlISShbZxM6DAazyLcG4Rr1nHAo241pw/GqIAugZnApVU2gyFCpGTFyR UNcW1x0rdVF2Hl1ySnKUM+jeDANrX2NuhXpanRYU= Message-ID: Subject: Re: [PATCH v2] tracing, synthetic events: Replace buggy strcat() with seq_buf operations From: Tom Zanussi To: Steven Rostedt Cc: LKML , Ingo Molnar , Masami Hiramatsu , Andrew Morton , Dan Carpenter Date: Mon, 26 Oct 2020 13:49:56 -0500 In-Reply-To: <20201026125314.705e53ba@gandalf.local.home> References: <20201026125314.705e53ba@gandalf.local.home> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.28.5-0ubuntu0.18.04.1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Steve, On Mon, 2020-10-26 at 12:53 -0400, Steven Rostedt wrote: > From: "Steven Rostedt (VMware)" > > 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) > --- > > 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 Tested-by: Tom Zanussi > 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; >