Date:   Mon, 4 Mar 2019 14:05:01 +0100
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     kbuild test robot <lkp@...el.com>
Cc:     kbuild-all@...org, linux-m68k <linux-m68k@...ts.linux-m68k.org>,
        Arnd Bergmann <arnd@...db.de>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Finn Thain <fthain@...egraphics.com.au>
Subject: Re: [m68k:master 1174/1174] arch/m68k/include/asm/string.h:72:25:
 warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7]

On Mon, Mar 4, 2019 at 1:44 PM kbuild test robot <lkp@...el.com> wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k.git master
> head:   e223cadc191661c67cb419b3a53c7854ecc39e8b
> commit: e223cadc191661c67cb419b3a53c7854ecc39e8b [1174/1174] Merge tag 'v5.0'
> config: m68k-allmodconfig (attached as .config)
> compiler: m68k-linux-gnu-gcc (Debian 8.2.0-11) 8.2.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         git checkout e223cadc191661c67cb419b3a53c7854ecc39e8b
>         # save the attached .config to linux build tree
>         GCC_VERSION=8.2.0 make.cross ARCH=m68k
>
> All warnings (new ones prefixed by >>):
>
>    In file included from include/linux/string.h:20,
>                     from include/linux/bitmap.h:9,
>                     from include/linux/nodemask.h:95,
>                     from include/linux/mmzone.h:17,
>                     from include/linux/gfp.h:6,
>                     from include/linux/umh.h:4,
>                     from include/linux/kmod.h:22,
>                     from include/linux/module.h:13,
>                     from drivers/nvme/target/admin-cmd.c:15:
>    In function 'memcpy_and_pad',
>        inlined from 'nvmet_execute_identify_ctrl' at drivers/nvme/target/admin-cmd.c:309:2:
> >> arch/m68k/include/asm/string.h:72:25: warning: '__builtin_memcpy' forming offset 8 is out of the bounds [0, 7] [-Warray-bounds]
>     #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
>                             ^~~~~~~~~~~~~~~~~~~~~~~~~
>    include/linux/string.h:456:3: note: in expansion of macro 'memcpy'
>       memcpy(dest, src, dest_len);
>       ^~~~~~
>
> vim +/__builtin_memcpy +72 arch/m68k/include/asm/string.h
>
> ea61bc46 Greg Ungerer 2010-09-07  69
> ea61bc46 Greg Ungerer 2010-09-07  70  #define __HAVE_ARCH_MEMCPY
> ea61bc46 Greg Ungerer 2010-09-07  71  extern void *memcpy(void *, const void *, __kernel_size_t);
> ea61bc46 Greg Ungerer 2010-09-07 @72  #define memcpy(d, s, n) __builtin_memcpy(d, s, n)
> ea61bc46 Greg Ungerer 2010-09-07  73
>
> :::::: The code at line 72 was first introduced by commit
> :::::: ea61bc461d09e8d331a307916530aaae808c72a2 m68k/m68knommu: merge MMU and non-MMU string.h
>
> :::::: TO: Greg Ungerer <gerg@...pgear.com>
> :::::: CC: Geert Uytterhoeven <geert@...ux-m68k.org>

Yep, that's a funny one, which I saw myself this morning, and decided
to dive into.
Apparently this is due 1) the recently added -ffreestanding and
2) the kernel version being shorter than 8 characters (indeed, it
didn't happen with allmodconfig on v5.0.0-rcX).

The offending code is:

        memcpy_and_pad(id->fr, sizeof(id->fr),
                       UTS_RELEASE, strlen(UTS_RELEASE), ' ');

with UTS_RELEASE being "5.0.0+", which calls into:

    static inline void memcpy_and_pad(void *dest, size_t dest_len,
                                      const void *src, size_t count, int pad)
    {
            if (dest_len > count) {

Of course this branch is taken, right?

                    memcpy(dest, src, count);
                    memset(dest + count, pad,  dest_len - count);
            } else
                    memcpy(dest, src, dest_len);
    }

This assembles to:

    .LC1:
            .string     "5.0.0+"

    | drivers/nvme/target/admin-cmd.c:311:      memcpy_and_pad(id->fr,
sizeof(id->fr),
            pea .LC1            |
            jsr strlen          |

Woops, gcc no longer optimizes this away, due to -ffreestanding.

    | drivers/nvme/target/admin-cmd.c:311:      memcpy_and_pad(id->fr,
sizeof(id->fr),
            lea (64,%a3),%a1    |, ret, _7
    | include/linux/string.h:452:       if (dest_len > count) {
            lea (16,%sp),%sp    |,
            moveq #7,%d1        |,
            cmp.l %d0,%d1       | _6,
            jcs .L53            |

And we end up with retaining both branches:

    | include/linux/string.h:453:               memcpy(dest, src, count);
            move.l %d0,-(%sp)   | _6,
            pea .LC1            |
            move.l %a1,-(%sp)   | _7,
            move.l %d0,-12(%fp) |,
            move.l %a1,-16(%fp) |,
            jsr memcpy          |
    | include/linux/string.h:454:               memset(dest + count,
pad,  dest_len - count);
            move.l -12(%fp),%d0 |,
            moveq #8,%d1        |,
            sub.l %d0,%d1       | _6,
            move.l %d1,-(%sp)   |,
            pea 32.w            |
            move.l -16(%fp),%a1 |,
            pea (%a1,%d0.l)             |
            jsr memset          |
            lea (24,%sp),%sp    |,
            jra .L54            |
    .L53:
    | include/linux/string.h:456:               memcpy(dest, src, dest_len);
            move.l .LC1,(%a1)   | MEM[(void *)"5.0.0+"], MEM[(void *)_7]
            move.l .LC1+4,4(%a1)        | MEM[(void *)"5.0.0+"], MEM[(void *)_7]

But given the warning, the compiler must have devised that taking the
second branch would read beyond the source buffer???

    .L54:


Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists