Bug 16004 - memcpy/strcpy: detect memory overlap and crash when error is detected
Summary: memcpy/strcpy: detect memory overlap and crash when error is detected
Status: NEW
Alias: None
Product: glibc
Classification: Unclassified
Component: string (show other bugs)
Version: unspecified
: P2 enhancement
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-10-05 11:29 UTC by Sergei Trofimovich
Modified: 2024-03-01 07:43 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Sergei Trofimovich 2013-10-05 11:29:49 UTC
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
Comment 1 Ondrej Bilka 2013-10-09 07:52:19 UTC
This is also possible to do with memstomp
Comment 2 paxdiablo 2014-06-23 03:11:42 UTC
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 :-)
Comment 3 Sergei Trofimovich 2014-06-23 07:19:15 UTC
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.
Comment 4 Sergei Trofimovich 2016-08-20 08:02:02 UTC
A while ago OpenBSD added simple overlap check to memcpy():

http://marc.info/?l=openbsd-ports&m=141666286130523&w=2
Comment 5 Paul Pluzhnikov 2016-08-22 15:34:07 UTC
FWIW, overlapping memcpy()ies are very common, and not always a "real" bug. E.g.

    void *p = allocate_and_init (10);
    ...
    void *q = realloc(p, 20);  // q may or may not == p.
    memcpy(q, p, 10);

I've seen examples of "not a real bug" where p!=q as well.

While in theory these all should be fixed, in practice end users may not be in a position to do so (e.g. prebuilt 3rd party library where the 3rd party doesn't exist anymore, or isn't willing or able to provide a fixed version).
Comment 6 jsm-csl@polyomino.org.uk 2016-08-22 22:05:23 UTC
GCC may generate code that assumes the p == q case works, so that a 
conditional isn't needed when converting a struct assignment to memcpy.  I 
think it would be reasonable for fortification to check for cases of 
overlap other than exact overlap.
Comment 7 Sergei Trofimovich 2016-08-24 12:15:57 UTC
(In reply to Paul Pluzhnikov from comment #5)
> FWIW, overlapping memcpy()ies are very common, and not always a "real" bug.
> E.g.
> 
>     void *p = allocate_and_init (10);
>     ...
>     void *q = realloc(p, 20);  // q may or may not == p.
>     memcpy(q, p, 10);

This one looks like a scary use-after-free. I'd expect multithreaded programs
to occasionally  get garbage data in q from p reused by other thread. Or a SIGSEGGV
if 16-byte bucket containing only p gets munmap()ed.

> I've seen examples of "not a real bug" where p!=q as well.

Can you share an example if it's not too complex? Does it have something to do with
struct assignment?

> While in theory these all should be fixed, in practice end users may not be
> in a position to do so (e.g. prebuilt 3rd party library where the 3rd party
> doesn't exist anymore, or isn't willing or able to provide a fixed version).

I agree some (almost) correct users might be broken. If there will be too many
broken users then changing glibc would make more harm and would have to be reverted.

I'd like a checking variant of memcpy() on my dev machines though.

If those prebuilt 3rdparty components are dynamic libraries/binaries then old
memcpy@GLIBC_2.2.5 / @GLIBC_2.14 symver should save them. If those
are static libraries user (dev) would have to pin to older memcpy symversion.
Or patch a binary to call memmove().

Having done such things in the past i'd say it's acceptable if very rare.
Comment 8 Paul Pluzhnikov 2016-08-24 16:56:04 UTC
(In reply to Sergei Trofimovich from comment #7)

> This one looks like a scary use-after-free.

You are right. Bad example. Most of the real ones involve swap() with itself.

> Can you share an example if it's not too complex? 

I went back to look (google ref: b/3186945), and didn't find any. I must have mis-remembered.


Related: https://sourceware.org/bugzilla/show_bug.cgi?id=12518
Comment 9 Florian Weimer 2016-08-24 18:41:01 UTC
(In reply to Paul Pluzhnikov from comment #8)
> (In reply to Sergei Trofimovich from comment #7)
> 
> > This one looks like a scary use-after-free.
> 
> You are right. Bad example. Most of the real ones involve swap() with itself.

And GCC generates such memcpy calls, even for valid C code:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=32667#c13