Re: [lkp-robot] [include/linux/string.h] 6974f0c455: kernel_BUG_at_lib/string.c

From: Masami Hiramatsu
Date: Thu Jul 20 2017 - 09:46:32 EST


Hi Linus,

On Wed, 19 Jul 2017 21:04:25 -0700
Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> 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
> <xiaolong.ye@xxxxxxxxx> 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 <mhiramat@xxxxxxxxxx>