Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964889AbdGTNqa (ORCPT ); Thu, 20 Jul 2017 09:46:30 -0400 Received: from mail.kernel.org ([198.145.29.99]:48644 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964840AbdGTNq2 (ORCPT ); Thu, 20 Jul 2017 09:46:28 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B14D722BCE Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=mhiramat@kernel.org Date: Thu, 20 Jul 2017 22:46:21 +0900 From: Masami Hiramatsu To: Linus Torvalds Cc: kernel test robot , Ananth N Mavinakayanahalli , Anil S Keshavamurthy , Daniel Micay , Kees Cook , Arnd Bergmann , Mark Rutland , Daniel Axtens , Rasmus Villemoes , Andy Shevchenko , Chris Metcalf , Thomas Gleixner , "H. Peter Anvin" , Ingo Molnar , Andrew Morton , LKML , LKP Subject: Re: [lkp-robot] [include/linux/string.h] 6974f0c455: kernel_BUG_at_lib/string.c Message-Id: <20170720224621.5182ab7cd2b3c9c337599c5a@kernel.org> In-Reply-To: References: <20170720014238.GH27396@yexl-desktop> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4055 Lines: 111 Hi Linus, On Wed, 19 Jul 2017 21:04:25 -0700 Linus Torvalds wrote: > Hmm. I wonder why the kernel test robot ends up having that annoying > line doubling for the dmesg. > > On Wed, Jul 19, 2017 at 6:42 PM, kernel test robot > wrote: > > > > FYI, we noticed the following commit: > > > > commit: 6974f0c4555e285ab217cee58b6e874f776ff409 ("include/linux/string.h: add the option of fortified string.h functions") > > > > caused below changes (please refer to attached dmesg/kmsg for entire log/backtrace): > > It does strike me that the fortify_panic() code once again makes life > unnecessarily hard for everybody by using "BUG()" > > What the hell is wrong with people? I feel; like I have to say this > multiple times for every single merge window, and sometimes in > between. > > BUG() and BUG_ON() are not acceptable debugging things. They kill the > machine. They make for bad debugging. > > > [ 8.134860] kernel BUG at lib/string.c:985! > > This is basically an entirely useless piece of completely > information-less garbage. > > It would have been much nicer if all the fortify_panic() calls had > instead used WARN_ONCE() with helpful pointers to what is going on. > > As it is, the full dmesg does show that > > detected buffer overflow in memcpy > > but since it was printed out separately, if comes before the "-- cut > here --" part and didn't get reported in the summary. > > > [ 8.134886] arch_prepare_optimized_kprobe+0xd5/0x171 > > It's apparently this: > > /* Copy arch-dep-instance from template */ > memcpy(buf, &optprobe_template_entry, TMPL_END_IDX); > > and looking at the code generation, the conditional in the fortify case is > > # ./include/linux/string.h:307: if (p_size < size || q_size < size) > cmpq $1, %r13 #, _14 > jbe .L109 #, > > but it's hard to tell whether that is p_size or q_size. One of them > must be ~0ul (or maybe both are 1) for it to have simplified to that > single conditional. > > So the fortify_string code has decided that only a single-byte (or > empty) memcpy is ok. > > And that, in turn, seems to be because we're copying from > optprobe_template_entry, which is declared as > > extern __visible kprobe_opcode_t optprobe_template_entry; > > so the fortify code decides it's a single character. Ah, right, that seems a single character, yes. > Does just changing all those things to be declared as arrays fix things? OK, and I also consider to replace the memcpy with probe_kernel_read() as I did it in commit ea1e34fc366b ("kprobes/x86: Use probe_kernel_read() instead of memcpy()"), since it reads memory in kernel-text area. (Hmm, why I didn't change this memcpy too ...?) It also fix that as a side effect. Thank you, > > IOW, a patch something like this white-space damaged mess: > > diff --git a/arch/x86/include/asm/kprobes.h b/arch/x86/include/asm/kprobes.h > index 34b984c60790..6cf65437b5e5 100644 > --- a/arch/x86/include/asm/kprobes.h > +++ b/arch/x86/include/asm/kprobes.h > @@ -52,10 +52,10 @@ typedef u8 kprobe_opcode_t; > #define flush_insn_slot(p) do { } while (0) > > /* optinsn template addresses */ > -extern __visible kprobe_opcode_t optprobe_template_entry; > -extern __visible kprobe_opcode_t optprobe_template_val; > -extern __visible kprobe_opcode_t optprobe_template_call; > -extern __visible kprobe_opcode_t optprobe_template_end; > +extern __visible kprobe_opcode_t optprobe_template_entry[]; > +extern __visible kprobe_opcode_t optprobe_template_val[]; > +extern __visible kprobe_opcode_t optprobe_template_call[]; > +extern __visible kprobe_opcode_t optprobe_template_end[]; > #define MAX_OPTIMIZED_LENGTH (MAX_INSN_SIZE + RELATIVE_ADDR_SIZE) > #define MAX_OPTINSN_SIZE \ > (((unsigned long)&optprobe_template_end - \ > > Hmm? > > Linus -- Masami Hiramatsu