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=-14.0 required=3.0 tests=BAYES_00,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS 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 08F21C1B08C for ; Thu, 15 Jul 2021 16:43:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E743F613C7 for ; Thu, 15 Jul 2021 16:43:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233502AbhGOQp5 (ORCPT ); Thu, 15 Jul 2021 12:45:57 -0400 Received: from mail-lf1-f43.google.com ([209.85.167.43]:37666 "EHLO mail-lf1-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232742AbhGOQp4 (ORCPT ); Thu, 15 Jul 2021 12:45:56 -0400 Received: by mail-lf1-f43.google.com with SMTP id b26so11013675lfo.4 for ; Thu, 15 Jul 2021 09:43:02 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=o0LG8VpqZSNIb8OFmmUa+XlaugtG0sYfQ1H76uCWCu8=; b=VTFHx/WUe69CjFEgfoT3Rv4SJ1KBtZpacihBg7evQluslqZISIlTV+uZkozujigMSE LdbrYpiZ16L5BCezzbczKCru/PcMJhWbr4MoiujLwO0BbN6w3h+hULOZb2bKn9lessyD 7R+rYBt3QFqjWwH3Cgqd2kIuDQPjH5w2sd7ejO0xBgX+0lS2+75wKvxbfUYnxqoKQBXo sjdgp1ZXf1tX4q8UV5vM3IwcYy7HyddDRVjYq//OXFbotmHnjMCHxT8U4aBNOqM0DXTC BbTTip2RKTqEQfVCoj62HZgEiZ4JnbIo3pFEuGsK7Oxunsp5mYqS070e8BxKfzVM6EB4 MGVw== X-Gm-Message-State: AOAM531GVwXUng4Ccetv7wXDKMMpI8Ac1oafH3epcMMhS0AaPLGEkXWb qg9rvRKvZlGAqMehXL+OrUGOwYNT5buURHnFDzd4lR94 X-Google-Smtp-Source: ABdhPJx6JntxzzycFp9ytICVmP508k+HP4hjE6kblc11AIGmSMf9hfGRnr9pGDGqudrQu/ppk7DHXURB8cWtyFzGQgg= X-Received: by 2002:a19:8c09:: with SMTP id o9mr4211739lfd.152.1626367381325; Thu, 15 Jul 2021 09:43:01 -0700 (PDT) MIME-Version: 1.0 References: <20210715000206.025df9d2@rorschach.local.home> In-Reply-To: <20210715000206.025df9d2@rorschach.local.home> From: Namhyung Kim Date: Thu, 15 Jul 2021 09:42:49 -0700 Message-ID: Subject: Re: [PATCH] tracing: Do no reference char * as a string in histograms To: Steven Rostedt Cc: LKML , Ingo Molnar , Andrew Morton , Tom Zanussi , Masami Hiramatsu , Tzvetomir Stoyanov Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Steve, On Wed, Jul 14, 2021 at 9:02 PM Steven Rostedt wrote: > > From: "Steven Rostedt (VMware)" > > The histogram logic was allowing events with char * pointers to be used as > normal strings. But it was easy to crash the kernel with: > > # echo 'hist:keys=filename' > events/syscalls/sys_enter_openat/trigger > > And open some files, and boom! > > BUG: unable to handle page fault for address: 00007f2ced0c3280 > #PF: supervisor read access in kernel mode > #PF: error_code(0x0000) - not-present page > PGD 1173fa067 P4D 1173fa067 PUD 1171b6067 PMD 1171dd067 PTE 0 > Oops: 0000 [#1] PREEMPT SMP > CPU: 6 PID: 1810 Comm: cat Not tainted 5.13.0-rc5-test+ #61 > Hardware name: Hewlett-Packard HP Compaq Pro 6300 SFF/339A, BIOS K01 > v03.03 07/14/2016 > RIP: 0010:strlen+0x0/0x20 > Code: f6 82 80 2a 0b a9 20 74 11 0f b6 50 01 48 83 c0 01 f6 82 80 2a 0b > a9 20 75 ef c3 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 <80> 3f 00 74 > 10 48 89 f8 48 83 c0 01 80 38 00 75 f7 48 29 f8 c3 > > RSP: 0018:ffffbdbf81567b50 EFLAGS: 00010246 > RAX: 0000000000000003 RBX: ffff93815cdb3800 RCX: ffff9382401a22d0 > RDX: 0000000000000100 RSI: 0000000000000000 RDI: 00007f2ced0c3280 > RBP: 0000000000000100 R08: ffff9382409ff074 R09: ffffbdbf81567c98 > R10: ffff9382409ff074 R11: 0000000000000000 R12: ffff9382409ff074 > R13: 0000000000000001 R14: ffff93815a744f00 R15: 00007f2ced0c3280 > FS: 00007f2ced0f8580(0000) GS:ffff93825a800000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007f2ced0c3280 CR3: 0000000107069005 CR4: 00000000001706e0 > Call Trace: > event_hist_trigger+0x463/0x5f0 > ? find_held_lock+0x32/0x90 > ? sched_clock_cpu+0xe/0xd0 > ? lock_release+0x155/0x440 > ? kernel_init_free_pages+0x6d/0x90 > ? preempt_count_sub+0x9b/0xd0 > ? kernel_init_free_pages+0x6d/0x90 > ? get_page_from_freelist+0x12c4/0x1680 > ? __rb_reserve_next+0xe5/0x460 > ? ring_buffer_lock_reserve+0x12a/0x3f0 > event_triggers_call+0x52/0xe0 > ftrace_syscall_enter+0x264/0x2c0 > syscall_trace_enter.constprop.0+0x1ee/0x210 > do_syscall_64+0x1c/0x80 > entry_SYSCALL_64_after_hwframe+0x44/0xae > > Where it triggered a fault on strlen(key) where key was the filename. > > The reason is that filename is a char * to user space, and the histogram > code just blindly dereferenced it, with obvious bad results. > > I originally tried to use strncpy_from_user/kernel_nofault() but found > that there's other places that its dereferenced and not worth the effort. > > Just do not allow "char *" to act like strings. It'd lose convenience and expressiveness but it's hard to argue if it crashes the kernel. So, Acked-by: Namhyung Kim Thanks, Namhyung > > Cc: stable@vger.kernel.org > Fixes: 79e577cbce4c4 ("tracing: Support string type key properly") > Signed-off-by: Steven Rostedt (VMware) > --- > kernel/trace/trace_events_hist.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/trace/trace_events_hist.c b/kernel/trace/trace_events_hist.c > index 0207aeed31e6..16a9dfc9fffc 100644 > --- a/kernel/trace/trace_events_hist.c > +++ b/kernel/trace/trace_events_hist.c > @@ -1689,7 +1689,9 @@ static struct hist_field *create_hist_field(struct hist_trigger_data *hist_data, > if (WARN_ON_ONCE(!field)) > goto out; > > - if (is_string_field(field)) { > + /* Pointers to strings are just pointers and dangerous to dereference */ > + if (is_string_field(field) && > + (field->filter_type != FILTER_PTR_STRING)) { > flags |= HIST_FIELD_FL_STRING; > > hist_field->size = MAX_FILTER_STR_VAL; > @@ -4495,8 +4497,6 @@ static inline void add_to_key(char *compound_key, void *key, > field = key_field->field; > if (field->filter_type == FILTER_DYN_STRING) > size = *(u32 *)(rec + field->offset) >> 16; > - else if (field->filter_type == FILTER_PTR_STRING) > - size = strlen(key); > else if (field->filter_type == FILTER_STATIC_STRING) > size = field->size; > > -- > 2.31.1 >