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

[Linus Torvalds]

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.

Does just changing all those things to be declared as arrays fix things?

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