[Bug libc/16004] New: memcpy/strcpy: detect memory overlap and crash when error is detected

[Bug libc/16004] New: memcpy/strcpy: detect memory overlap and crash when error is detected

[slyfox at inbox dot ru]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

            Bug ID: 16004
           Summary: memcpy/strcpy: detect memory overlap and crash when
                    error is detected
           Product: glibc
           Version: unspecified
            Status: NEW
          Severity: enhancement
          Priority: P2
         Component: libc
          Assignee: unassigned at sourceware dot org
          Reporter: slyfox at inbox dot ru
                CC: drepper.fsp at gmail dot com

Recently amount of bugs regarding to memcpy() copy ordering grown.
I guess it's due to new memcpy@GLIBC_2.14 on new CPUs.

What scary is bugs are always data corruptions and almost never
immediate application crashes (usually seen way later, that in
actual memcpy call). Thus they live very long time unnoticed.

Sometimes bug exhibits only on a subset of hardware (on core i7,
but not core2, same libc) or even oncertain heap. I guess it's
due to different semantics of memcpy IFUNC implelentations
on different host CPUs (depend on linear address alignment).

I'd like to ask to put memory overlap checking right into:
- memcpy implementations themselves before calling assembly implementation
- if above is too heavyweight, then do it only in _FORTIFY_SOURCE mode
- if above is too heavyweight, then add some variable/knob to allow
  putting slow checky memcpy at IFUNC startup time.

Same goes for strcpy. Sometimes people do silly things, like
    strcpy (some_str, some_str + strlen ("pref"));
and get data corruption.

Thank you!

Some data corruptions I've found myself:
  https://bugs.gentoo.org/show_bug.cgi?id=486984 - aircrack-ng's packet parsers
bug with memcpy to move packes for some bytes.
 
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=3387206f26e1b48703e810175b98611a4fd8e8ea
- filesystem corruption bug. It's a kernel patch, but usermode linux can be
forced to use libc's memcpy to find such bugs easier.
 
https://github.com/MidnightCommander/mc/commit/e48cb7c89ff3e54de70130a3de2136a9902a023d
- occasional data corruption of final filepaths

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[neleai at seznam dot cz]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

Ondrej Bilka <neleai at seznam dot cz> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |neleai at seznam dot cz

--- Comment #1 from Ondrej Bilka <neleai at seznam dot cz> ---
This is also possible to do with memstomp

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
              Flags|                            |security-

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[allachan at au1 dot ibm.com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

paxdiablo <allachan at au1 dot ibm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |allachan at au1 dot ibm.com

--- Comment #2 from paxdiablo <allachan at au1 dot ibm.com> ---
Hmmm.

"Sometimes people do silly things". I think the fix here may be to convince
people to stop doing those silly things :-)

ISO C11 (and earlier) specifically call out the possibility of overlap
problems, for both memcpy() and strcpy():

"If copying takes place between objects that overlap, the behavior is
undefined."

What you seem to be proposing is to slow down all copy operations to handle
things people shouldn't be doing in the first place. I'm of the opinion that C
bods who need this level of hand-holding should consider moving to C++ (I'm not
dissing C++ here but people should learn the tools they use, or get tools
better suited to them). Putting "safety" into one clib implementation will not
help you when you have to use another.

C already has a perfectly good library function for overlapping memory copies,
to wit, memmove(). I'd be more inclined to ask ISO WG14 to introduce a new
strmove() call for doing the same for C strings, or just use memmove() with
strlen() to achieve the same result, something like:

char *myStrMove (
  char * restrict s1,
  char * restrict s2)
{
  memmove (s1, s2, strlen (s2) + 1);
  return s1;
}

Don't take this as an attack, you've done exactly the right thing in your
layered approach for solutions ("if too heavy, do something else"). I just
don't think this should be changed for ALL XXXcpy() calls since it'll have a
performance hit.

By all means, if there's a different XXXcpy() we can use in those fortify
modes, I'd be happy. Though it's still going to crash, at least it'll fail fast
hopefully before corrupting data. Not sure how good that's going to make users
feel since they'll still have lost their unsaved work :-)

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[slyfox at inbox dot ru]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

--- Comment #3 from Sergei Trofimovich <slyfox at inbox dot ru> ---
Well, I think:
* for most of simple (stack-to-stack) 'memcpy's issues by gcc compiler can
prove there is no overlap and call just an unsafe memcpy
* for big chunks to copy one check won't hurt performance
* for small unaligned chunks memcpy is already inefficient

Here is another real-world data corruption:
https://bugs.gentoo.org/show_bug.cgi?id=486984

This time it "just" required patched toolchain and
brand new CPU to notice a bug in test suite.
Before that fix it "only" corrupted real data on
new CPUs but passed test suite.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug libc/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[fweimer at redhat dot com]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

Florian Weimer <fweimer at redhat dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |fweimer at redhat dot com

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[jsm28 at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

Joseph Myers <jsm28 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|libc                        |string

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug string/16004] memcpy/strcpy: detect memory overlap and crash when error is detected

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=16004

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=32667
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.