[Bug target/94158] New: Expanded strlen causes out-of-bounds read on AMD64 target

[Bug target/94158] New: Expanded strlen causes out-of-bounds read on AMD64 target

[parker@cyber-itl.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

            Bug ID: 94158
           Summary: Expanded strlen causes out-of-bounds read on AMD64
                    target
           Product: gcc
           Version: 9.2.1
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: parker@cyber-itl.org
  Target Milestone: ---

On AMD64/Linux, When strlen() is expanded, if the argument to strlen comes from
a source like strdup() or other functions returning allocator generated
pointers, the expanded strlen() makes an assumption about the alignment of the
char array.

Expanded strlen():

  callq  0x4011e0 <strdup>

  mov    %rax,%rsi
  mov    %rax,%rdx

  mov    (%rdx),%ecx
  add    $0x4,%rdx
  lea    -0x1010101(%rcx),%eax
  not    %ecx
  and    %ecx,%eax
  and    $0x80808080,%eax
  je     0x40109a <main+58>

If the input argument is not 4-byte aligned, the "mov (%rdx),%ecx" can read up
to 3 bytes off the end of the allocated array.

Testing with GCC 9.2.1 this expanding behavior can be trigger with -O2 and -O3.
 On git HEAD (c56871dd15a258c8775740194e6ed960a1f3d850) it requires the
'-minline-all-stringops' flag and -O2 / -O3.

Reproduction:



#include <stdio.h>
#include <sys/mman.h>

#define PAGE_SIZE       4096
#define ALIGN_SIZE(x) (((x) + PAGE_SIZE - 1) & ~(PAGE_SIZE - 1))

char *strdup(const char *str) {
    size_t size = __builtin_strlen(str) + 1;
    size_t len = ALIGN_SIZE(size);
    char *ptr = (char *) mmap(NULL, len + PAGE_SIZE, PROT_READ | PROT_WRITE,
MAP_ANONYMOUS | MAP_PRIVATE, 0, 0);
    munmap(&ptr[len], PAGE_SIZE);
    char *dest = ptr + PAGE_SIZE - size;
    __builtin_memcpy(dest, str, size);
    return dest;
}

int main(int argc, char **argv) {
    char blah[] = "AAAABBBBCCCCDD";
    char *dest = strdup(blah);
    __builtin_printf("%s %ld\n", dest, __builtin_strlen(dest));

    return 0;
}




Newer than 9.2.1:
gcc -minline-all-stringops -O2 -o strlen-crash ./strlen-crash.c

9.2.1 and below:
gcc -O2 -o strlen-crash ./strlen-crash.c


./strlen-crash # Should SIGSEGV on the 'mov (%rdx),%ecx' instruction, if
expanded.


GCC -v output:
Using built-in specs.
COLLECT_GCC=gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/x86_64-pc-linux-gnu/9.2.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: /build/gcc/src/gcc/configure --prefix=/usr --libdir=/usr/lib
--libexecdir=/usr/lib --mandir=/usr/share/man --infodir=/usr/share/info
--with-pkgversion='Arch Linux 9.2.1+20200130-2'
--with-bugurl=https://bugs.archlinux.org/
--enable-languages=c,c++,ada,fortran,go,lto,objc,obj-c++,d --enable-shared
--enable-threads=posix --with-system-zlib --with-isl --enable-__cxa_atexit
--disable-libunwind-exceptions --enable-clocale=gnu --disable-libstdcxx-pch
--disable-libssp --enable-gnu-unique-object --enable-linker-build-id
--enable-lto --enable-plugin --enable-install-libiberty
--with-linker-hash-style=gnu --enable-gnu-indirect-function --enable-multilib
--disable-werror --enable-checking=release --enable-default-pie
--enable-default-ssp --enable-cet=auto gdc_include_dir=/usr/include/dlang/gdc
Thread model: posix
gcc version 9.2.1 20200130 (Arch Linux 9.2.1+20200130-2)


GCC -v from git HEAD (c56871dd15a258c8775740194e6ed960a1f3d850):
Using built-in specs.
COLLECT_GCC=./bin/gcc
COLLECT_LTO_WRAPPER=/tmp/gcc/build-bins/libexec/gcc/x86_64-pc-linux-gnu/10.0.1/lto-wrapper
Target: x86_64-pc-linux-gnu
Configured with: ../configure --prefix=/tmp/gcc/build-bins/
--enable-languages=c,c++ --disable-bootstrap --disable-multilib
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.0.1 20200312 (experimental) (GCC)

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
A pointer returned from strdup has to be valid to be able pass to free.

Your testcase makes that invalid.

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-03-12

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Also aligned_alloc normally does not allow alignment of 1.

So GCC is doing the correct thing.

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[parker@cyber-itl.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

--- Comment #3 from Parker Thompson <parker@cyber-itl.org> ---
(In reply to Andrew Pinski from comment #2)
> Also aligned_alloc normally does not allow alignment of 1.
> 
> So GCC is doing the correct thing.

The replacement of strdup here is just to illustrate the issue with expansion
alignment of strlen() by forcing a crash.

I encountered this issue when working with a custom malloc replacement that
would enforce out-of-bounds read checks. Using the same reproduction with clang
did not produce a crash / oob-read.

As for alloc alignment, glibc strdup() does not use aligned_alloc, just malloc.
 Which by my read of the spec does not guarantee alignment.

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
         Resolution|---                         |INVALID

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Parker Thompson from comment #3)
> As for alloc alignment, glibc strdup() does not use aligned_alloc, just
> malloc.  Which by my read of the spec does not guarantee alignment.

malloc requires alignement of alignof(max_align_t) (definition in the newest C
and C++ standards) but beforehand it was required to be aligned enough to
support all standard types.

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #4)
> (In reply to Parker Thompson from comment #3)
> > As for alloc alignment, glibc strdup() does not use aligned_alloc, just
> > malloc.  Which by my read of the spec does not guarantee alignment.
> 
> malloc requires alignement of alignof(max_align_t) (definition in the newest
> C and C++ standards) but beforehand it was required to be aligned enough to
> support all standard types.

C11 is where the definition changes to use max_align_t.

https://en.cppreference.com/w/c/types/max_align_t

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC assumes pointers returned by malloc are at least MALLOC_ABI_ALIGNMENT bytes
aligned.  That is because:
"The pointer returned if the allocation succeeds is suitably aligned so that it
may be assigned to a pointer to any type of object and then used to access such
an object or an array of such objects in the space allocated (until the space
is explicitly deallocated)."
glibc normally guarantees 2 * sizeof (void *) alignment, GCC by default assumes
just a wordsize alignment.
So, if your malloc replacement doesn't provide such alignment, it is incorrect.

[Bug target/94158] Expanded strlen causes out-of-bounds read on AMD64 target

[parker@cyber-itl.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94158

--- Comment #7 from Parker Thompson <parker@cyber-itl.org> ---
(In reply to Jakub Jelinek from comment #6)
> GCC assumes pointers returned by malloc are at least MALLOC_ABI_ALIGNMENT
> bytes aligned.  That is because:
> "The pointer returned if the allocation succeeds is suitably aligned so that
> it may be assigned to a pointer to any type of object and then used to
> access such an object or an array of such objects in the space allocated
> (until the space is explicitly deallocated)."
> glibc normally guarantees 2 * sizeof (void *) alignment, GCC by default
> assumes just a wordsize alignment.
> So, if your malloc replacement doesn't provide such alignment, it is
> incorrect.

Ahhh I see, I apologize for my misunderstanding.