Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 5 Mar 2019 13:56:00 +1100 (AEDT)
From:   Finn Thain <fthain@telegraphics.com.au>
To:     Geert Uytterhoeven <geert@linux-m68k.org>
cc:     kbuild test robot <lkp@intel.com>, kbuild-all@01.org,
        linux-m68k <linux-m68k@lists.linux-m68k.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
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]
In-Reply-To: <CAMuHMdX365qmWiii=gQLADpW49EMkdDrVJDPWNBpAZuZM0WQFQ@mail.gmail.com>
Message-ID: <alpine.LNX.2.21.1903051154200.135@nippy.intranet>
References: <201903042049.npxcZzps%fengguang.wu@intel.com> <CAMuHMdX365qmWiii=gQLADpW49EMkdDrVJDPWNBpAZuZM0WQFQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 4 Mar 2019, Geert Uytterhoeven wrote:

> On Mon, Mar 4, 2019 at 1:44 PM kbuild test robot <lkp@intel.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@snapgear.com>
> > :::::: CC: Geert Uytterhoeven <geert@linux-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?
> 

Apparently that's not known at compile time. The compiler knows dest_len 
is 8 but apparently doesn't know what count is, because -ffreestanding 
means it can't evaluate strlen(UTS_RELEASE). If you change that to 
__builtin_strlen(UTS_RELEASE), the problem goes away.

>                     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???
> 

Looks bogus to me.

If you change memcpy to __builtin_memcpy, then we avoid the macro and the 
warning changes to,

./include/linux/string.h:456:3: warning: '__builtin_memcpy' forming offset [7, 8] is out of the bounds [0, 6] [-Warray-bounds]
   __builtin_memcpy(dest, src, dest_len);

The compiler has nothing to complain about here. dest is known to be 
id->fr and dest_len is known to be sizeof(id->fr).

The error message indicates that gcc has applied the bounds [0, 6] to dest 
when in fact those are the bounds for src.

-- 

>     .L54:
> 
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
>