Skip to content

perform size checks on memcpy/memmove/memset #252

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SkewedZeppelin
Copy link

@SkewedZeppelin SkewedZeppelin commented Mar 21, 2025

for #231

  • largely works system wide on fedora 41 & 42

task list

  • gcc compiler doing weird things
    • no longer an issue with using real underlying functions
  • clang compiler doing weird things
    • it runs now, but size checks are always max
  • whole object size checks (fast path)
  • object remaining size checks (non-fast path)
  • optimized assembly functions
  • memcpy
  • memccpy
    • handle common read overflow case
  • memmove
  • memset
  • wmemcpy
  • wmemmove
  • wmemset
  • bypass overrides for self
  • licensing
  • makefile bits
  • readme
    • could be expanded on
  • test case for memcpy
    • overlap test
  • test case for memccpy
  • test case for memmove
  • test case for memset
  • test case for wmemcpy
  • test case for wmemmove
  • test case for wmemset
  • run all the test cases
    • the feature is default disabled so they can't be run without failing
  • figure out why test cases fail under CI when enabled
    • they all pass on my end
    • still not working on latest patchset
  • figure out why so many gnome apps crash
    • fatal allocator error: invalid malloc_object_size
    • conflict with gjs/mozjs?
    • crashes under f42, but not f41: clocks, calculator, baobab, fileroller, logs
    • crashes under f41, but not f42: gnome-shell when clicking top bar controls
    • can't reproduce anymore, unsure why
  • figure out how to handle chromium/electron crash/conflict
    • can't reproduce anymore, only happens on fast path
  • figure out if it is possible to use the real underlying functions for better per-arch performance
    • dlsym doesn't seem to work with all program such as mutter-x11-frames
      • can't reproduce anymore
    • this doesn't necessarily pull from libc, but can pull from other libraries
    • it feels unsafe

compared to others:

  • redfat: memchr, memcmp, memcpy, memmove, memrchr, memset, strcasecmp, strcasestr, strcat, strchr, strchrnul, strcmp, strcpy, strlen, strncasecmp, strncat, strncmp, strncpy, strnlen, strrchr, strstr
  • this patchset: bcopy, memccpy, memcpy, memmove, mempcpy, memset, swab, wmemcpy, wmemmove, wmempcpy, wmemset
  • isoalloc: memcpy, memmove, memset
  • snmalloc: memcpy, and previously memmove (disabled due to issue found by fuzzing)

@SkewedZeppelin SkewedZeppelin marked this pull request as draft March 21, 2025 22:51
@SkewedZeppelin SkewedZeppelin force-pushed the memcpy-sanity branch 2 times, most recently from 39b1b13 to b2fbe6e Compare March 22, 2025 00:30
@SkewedZeppelin SkewedZeppelin changed the title override memcpy and perform sanity checks override memcpy and perform size checks Mar 22, 2025
@SkewedZeppelin
Copy link
Author

@thestinger is it expected that malloc_object_size would return negative values?

@thestinger
Copy link
Member

@SkewedZeppelin It returns size_t so it can't be negative. When it doesn't know the answer, it returns SIZE_MAX. Are you treating it as signed somewhere?

@SkewedZeppelin
Copy link
Author

size_t size = region == NULL ? SIZE_MAX : region->size;
is negative sometimes

@thestinger
Copy link
Member

@SkewedZeppelin It's a size_t, it can't be negative. SIZE_MAX is the maximum size_t value. You must be printing it as a signed integer where it would be -1.

@SkewedZeppelin
Copy link
Author

apologies I'm dumb and was using %ld not %lu

@thestinger
Copy link
Member

@SkewedZeppelin It's not particularly important but you should use %zu for size_t.

@thestinger
Copy link
Member

It matters on Windows where long is 32-bit on 64-bit but could at least theoretically be the case elsewhere.

@SkewedZeppelin
Copy link
Author

SkewedZeppelin commented Mar 22, 2025

is it worth it to zero the remainder of dst? my only issue is that sometimes it is close to size_max/unknown

@thestinger
Copy link
Member

@SkewedZeppelin That wouldn't be safe since it's not known what they're doing with it. They could be intentionally only copying to part of it. It can be a copy to the middle of it, etc.

@SkewedZeppelin SkewedZeppelin changed the title override memcpy and perform size checks perform size checks on memcpy/memmove/memset Mar 22, 2025
@SkewedZeppelin SkewedZeppelin marked this pull request as ready for review March 22, 2025 04:07
@SkewedZeppelin SkewedZeppelin force-pushed the memcpy-sanity branch 9 times, most recently from b8cdf16 to 849055d Compare March 22, 2025 09:01
@SkewedZeppelin SkewedZeppelin force-pushed the memcpy-sanity branch 2 times, most recently from 4461652 to e38da0d Compare March 23, 2025 01:19
@SkewedZeppelin SkewedZeppelin marked this pull request as draft March 23, 2025 02:33
@SkewedZeppelin
Copy link
Author

SkewedZeppelin commented Mar 26, 2025

@cgzones thanks!

why go back to using self functions instead of host?

also I did have a revision somewhere above that had musl's assembly functions for speed

and how did you prevent the compiler from optimizing out the functions?

@SkewedZeppelin SkewedZeppelin force-pushed the memcpy-sanity branch 3 times, most recently from 70a34d5 to 6e8ca6b Compare March 26, 2025 10:09
@cgzones
Copy link
Contributor

cgzones commented Mar 26, 2025

why go back to using self functions instead of host?

i can't follow, can you explain?

also I did have a revision somewhere above that had musl's assembly functions for speed

i wanted to remain architecture agnostic

and how did you prevent the compiler from optimizing out the functions?

simple programs like this one will get optimized and succeed:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

int main() {
        char *src = malloc(1024);
        char *dst = malloc(256);
        memset(src, 's', 1024);
        memset(dst, 'd', 256);

        memcpy(dst, src, 1024);

        printf("%#x %#x %#x  ['s'=%#x, 'd'=%#x]\n", dst[24], dst[255], dst[511], 's', 'd');

        printf("success?\n");

        return EXIT_SUCCESS;
}

but when using non compile-time constants it seems to work:

#include <string.h>
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char *argv[]) {
        if (argc != 2) {
                fprintf(stderr, "usage: %s <size>\n", argv[0]);
                return EXIT_FAILURE;
        }

        int size = atoi(argv[1]);

        printf("using size %d...\n", size);

        char *src = malloc(1024);
        char *dst = malloc(256);
        memset(src, 's', 1024);
        memset(dst, 'd', 256);

        memcpy(dst, src, size);

        printf("%#x %#x %#x  ['s'=%#x, 'd'=%#x]\n", dst[24], dst[255], dst[511], 's', 'd');

        printf("success?\n");

        return EXIT_SUCCESS;
}

@SkewedZeppelin
Copy link
Author

SkewedZeppelin commented Mar 26, 2025

i can't follow, can you explain?

as in not using dlsym to get the real functions

agnostic

should I add them back?

simple programs like this

I meant I don't understand why the compiler isn't optimizing out memcpy like I mentioned here: #252 (comment)

@cgzones
Copy link
Contributor

cgzones commented Mar 26, 2025

i can't follow, can you explain?

as in not using dlsym to get the real functions

i didn't seem to work with rustc and vscode, probably due some bundled libc?

@SkewedZeppelin
Copy link
Author

@thestinger
any insight as to why malloc_object_size() is almost always returning SIZE_MAX when compiled with clang?

@thestinger
Copy link
Member

@SkewedZeppelin It shouldn't matter since it's done dynamically. Can you give an example?

@SkewedZeppelin SkewedZeppelin force-pushed the memcpy-sanity branch 5 times, most recently from 1100900 to c7141d1 Compare March 27, 2025 19:39
@SkewedZeppelin
Copy link
Author

SkewedZeppelin commented Mar 27, 2025

@cgzones
done, thanks! verified with the newly added test cases for them too.

edit: testing programs again:

org.openrgb.OpenRGB.desktop[4758]: fatal allocator error: wmemcpy overlap
audacity[4549]: fatal allocator error: wmemcpy overlap

broken check? or something doing a silly?
edit 2: valgrind has no support for detecting wmemcpy overlap, only memcpy overlap. neither of them make direct calls to wmemcpy, no idea what library is doing so.

@cgzones
Copy link
Contributor

cgzones commented Mar 27, 2025

org.openrgb.OpenRGB.desktop[4758]: fatal allocator error: wmemcpy overlap
audacity[4549]: fatal allocator error: wmemcpy overlap

broken check? or something doing a silly?

https://sources.debian.org/src/wxwidgets3.2/3.2.6%2Bdfsg-2/include/wx/string.h/#L1180

// copy ctor
wxString(const wxString& stringSrc) : m_impl(stringSrc.m_impl) { }

Maybe this copy constructor should have a self-copy guard?

backtrace
#4  0x00007ffff7fb0109 in fatal_error (s=s@entry=0x7ffff7fb71e1 "wmemcpy overlap") at /var/home/christian/Coding/workspaces/hardened_malloc/util.c:42
        prefix = 0x7ffff7fb7000 "fatal allocator error: "
#5  0x00007ffff7fb0b17 in wmemcpy(wchar_t * __restrict__, const wchar_t * __restrict__, size_t) (dst=0x73d76f3061c0 L"", src=src@entry=0x73d76f306240 L"wxConfigBase", len=len@entry=12)
    at /var/home/christian/Coding/workspaces/hardened_malloc/h_malloc.c:1928
        lenAdj = 48
#6  0x00007ffff67bcc9b in wmemcpy (__s1=<optimized out>, __s2=0x73d76f306240 L"wxConfigBase", __n=12) at /usr/include/x86_64-linux-gnu/bits/wchar2.h:30
#7  std::char_traits<wchar_t>::copy (__s1=<optimized out>, __s2=0x73d76f306240 L"wxConfigBase", __n=12) at /usr/include/c++/14/bits/char_traits.h:558
#8  std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_S_copy (__d=<optimized out>, __s=0x73d76f306240 L"wxConfigBase", __n=12)
    at /usr/include/c++/14/bits/basic_string.h:435
#9  std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_S_copy_chars (__p=<optimized out>, __k1=0x73d76f306240 L"wxConfigBase", __k2=<optimized out>)
    at /usr/include/c++/14/bits/basic_string.h:483
#10 std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::_M_construct<wchar_t*> (this=0x73d76f3060c0, __beg=0x73d76f306240 L"wxConfigBase", __end=<optimized out>)
    at /usr/include/c++/14/bits/basic_string.tcc:247
        __dnew = 12
        __guard = {_M_guarded = <optimized out>}
        __dnew = <optimized out>
        __guard = {_M_guarded = <optimized out>}
#11 std::__cxx11::basic_string<wchar_t, std::char_traits<wchar_t>, std::allocator<wchar_t> >::basic_string (this=0x73d76f3060c0, __str=<optimized out>) at /usr/include/c++/14/bits/basic_string.h:556
#12 wxString::wxString (this=0x73d76f3060c0, stringSrc=<optimized out>) at ./include/wx/string.h:1180
#13 wxHashTableBase_Node::wxHashTableBase_Node (this=0x73c607192f00, key=<optimized out>, value=<optimized out>, table=<optimized out>) at ./src/common/hash.cpp:39
#14 0x00007ffff67bcf7a in wxHashTableBase::DoPut (this=this@entry=0x73e7450d7730, key=..., hash=<optimized out>, data=data@entry=0x7ffff6963420 <wxConfigBase::ms_classInfo>) at ./src/common/hash.cpp:172
        bucket = 216
        node = <optimized out>
#15 0x00007ffff67d793c in wxHashTable::Put (this=0x73e7450d7730, value=..., object=0x7ffff6963420 <wxConfigBase::ms_classInfo>) at ./include/wx/hash.h:206
#16 wxClassInfo::Register (this=this@entry=0x7ffff6963420 <wxConfigBase::ms_classInfo>) at ./src/common/object.cpp:241
        entry = 1
        classTable = 0x73e7450d7730
#17 0x00007ffff672dd71 in wxClassInfo::wxClassInfo
    (this=0x7ffff6963420 <wxConfigBase::ms_classInfo>, className=0x7ffff68dbd00 L"wxConfigBase", baseInfo1=<optimized out>, baseInfo2=0x0, size=128, ctor=0x0) at ./include/wx/rtti.h:59
#18 __static_initialization_and_destruction_0 () at ./src/common/config.cpp:71
#19 _GLOBAL__sub_I_config.cpp(void) () at ./src/common/config.cpp:600
--Type <RET> for more, q to quit, c to continue without paging--c
#20 0x00007ffff7fcbf4e in call_init (l=<optimized out>, argc=1, argv=0x7fffffffd9c8, env=0x7fffffffd9d8) at ./elf/dl-init.c:74
        j = 0
        jm = <optimized out>
        addrs = <optimized out>
        init_array = <optimized out>
        init_array = <optimized out>
        j = <optimized out>
        jm = <optimized out>
        addrs = <optimized out>
#21 call_init (l=<optimized out>, argc=1, argv=0x7fffffffd9c8, env=0x7fffffffd9d8) at ./elf/dl-init.c:26
        init_array = <optimized out>
        j = <optimized out>
        jm = <optimized out>
        addrs = <optimized out>
#22 0x00007ffff7fcc01c in _dl_init (main_map=0x7ffff7ffe310, argc=1, argv=0x7fffffffd9c8, env=0x7fffffffd9d8) at ./elf/dl-init.c:121
        preinit_array = <optimized out>
        preinit_array_size = <optimized out>
        i = <optimized out>
#23 0x00007ffff7fe35f0 in _dl_start_user () at /lib64/ld-linux-x86-64.so.2

edit: hmm, the check should not trigger for these addresses dst=0x73d76f3061c0 L"", src=src@entry=0x73d76f306240 and a length of 48 (4 * 12)

@cgzones
Copy link
Contributor

cgzones commented Mar 27, 2025

diff --git a/h_malloc.c b/h_malloc.c
index 9db6dcc..1ea6224 100644
--- a/h_malloc.c
+++ b/h_malloc.c
@@ -1884,7 +1884,7 @@ EXPORT void *memcpy(void *restrict dst, const void *restrict src, size_t len) {
     if (unlikely(dst == src || len == 0)) {
         return dst;
     }
-    if (unlikely(dst < src + len && dst + len > src)) {
+    if (unlikely(dst < (src + len) && (dst + len) > src)) {
         fatal_error("memcpy overlap");
     }
     if (unlikely(len > malloc_object_size(src))) {
@@ -1924,8 +1924,8 @@ EXPORT wchar_t *wmemcpy(wchar_t *restrict dst, const wchar_t *restrict src, size
         return dst;
     }
     size_t lenAdj = len * sizeof(wchar_t);
-    if (unlikely(dst < src + lenAdj && dst + lenAdj > src)) {
+    if (unlikely(dst < (src + len) && (dst + len) > src)) {
         fatal_error("wmemcpy overlap");
     }
     if (unlikely(lenAdj > malloc_object_size(src))) {
         fatal_error("wmemcpy read overflow");

Those are not void pointers but wchar_t ones so one must not add bytes but number-of-items

@cgzones
Copy link
Contributor

cgzones commented Mar 30, 2025

Another function to cover could be memccpy(3):

patch
---
 Makefile                        |  4 +++-
 h_malloc.c                      | 16 ++++++++++++++
 include/h_malloc.h              |  1 +
 memccpy.c                       | 38 +++++++++++++++++++++++++++++++++
 musl.h                          |  1 +
 test/.gitignore                 |  4 ++++
 test/Makefile                   |  4 ++++
 test/memccpy_buffer_overflow.c  | 15 +++++++++++++
 test/memccpy_read_overflow.c    | 15 +++++++++++++
 test/memccpy_valid_mismatched.c | 15 +++++++++++++
 test/memccpy_valid_same.c       | 15 +++++++++++++
 test/test_smc.py                | 24 +++++++++++++++++++++
 12 files changed, 151 insertions(+), 1 deletion(-)
 create mode 100644 memccpy.c
 create mode 100644 test/memccpy_buffer_overflow.c
 create mode 100644 test/memccpy_read_overflow.c
 create mode 100644 test/memccpy_valid_mismatched.c
 create mode 100644 test/memccpy_valid_same.c

diff --git a/Makefile b/Makefile
index e3eee31..d18d0b3 100644
--- a/Makefile
+++ b/Makefile
@@ -41,7 +41,7 @@ LDFLAGS := $(LDFLAGS) -Wl,-O1,--as-needed,-z,defs,-z,relro,-z,now,-z,nodlopen,-z
 
 SOURCES := chacha.c h_malloc.c memory.c pages.c random.c util.c
 ifeq ($(CONFIG_BLOCK_OPS_CHECK_SIZE),true)
-    SOURCES += memcpy.c memmove.c memset.c wmemset.c
+    SOURCES += memcpy.c memccpy.c memmove.c memset.c wmemset.c
     BOSC_EXTRAS := musl.h
 endif
 OBJECTS := $(SOURCES:.c=.o)
@@ -142,6 +142,8 @@ $(OUT)/util.o: util.c util.h $(CONFIG_FILE) | $(OUT)
 
 $(OUT)/memcpy.o: memcpy.c musl.h $(CONFIG_FILE) | $(OUT)
 	$(COMPILE.c) -Wno-cast-align $(OUTPUT_OPTION) $<
+$(OUT)/memccpy.o: memccpy.c musl.h $(CONFIG_FILE) | $(OUT)
+	$(COMPILE.c) -Wno-cast-align $(OUTPUT_OPTION) $<
 $(OUT)/memmove.o: memmove.c musl.h $(CONFIG_FILE) | $(OUT)
 	$(COMPILE.c) -Wno-cast-align $(OUTPUT_OPTION) $<
 $(OUT)/memset.o: memset.c musl.h $(CONFIG_FILE) | $(OUT)
diff --git a/h_malloc.c b/h_malloc.c
index 5b7acb2..c99c0ca 100644
--- a/h_malloc.c
+++ b/h_malloc.c
@@ -1901,6 +1901,22 @@ EXPORT void *memcpy(void *restrict dst, const void *restrict src, size_t len) {
     return musl_memcpy(dst, src, len);
 }
 
+EXPORT void *memccpy(void *restrict dst, const void *restrict src, int value, size_t len) {
+    if (unlikely(dst == src || len == 0)) {
+        return dst;
+    }
+    if (unlikely(dst < (src + len) && (dst + len) > src)) {
+        fatal_error("memccpy overlap");
+    }
+    if (unlikely(len > malloc_object_size(src))) {
+        fatal_error("memccpy read overflow");
+    }
+    if (unlikely(len > malloc_object_size(dst))) {
+        fatal_error("memccpy buffer overflow");
+    }
+    return musl_memccpy(dst, src, value, len);
+}
+
 EXPORT void *memmove(void *dst, const void *src, size_t len) {
     if (unlikely(dst == src || len == 0)) {
         return dst;
diff --git a/include/h_malloc.h b/include/h_malloc.h
index ceca456..7974ff5 100644
--- a/include/h_malloc.h
+++ b/include/h_malloc.h
@@ -57,6 +57,7 @@ void h_free(void *ptr);
 
 #if CONFIG_BLOCK_OPS_CHECK_SIZE && !defined(HAS_ARM_MTE)
 void *memcpy(void *dst, const void *src, size_t len);
+void *memccpy(void *dst, const void *src, int value, size_t len);
 void *memmove(void *dst, const void *src, size_t len);
 void *memset(void *dst, int value, size_t len);
 wchar_t *wmemcpy(wchar_t *dst, const wchar_t *src, size_t len);
diff --git a/memccpy.c b/memccpy.c
new file mode 100644
index 0000000..e01a9eb
--- /dev/null
+++ b/memccpy.c
@@ -0,0 +1,38 @@
+#include "musl.h"
+
+/* Copied from musl libc version 1.2.5 licensed under the MIT license */
+
+#include <string.h>
+#include <stdint.h>
+#include <limits.h>
+
+#define ALIGN (sizeof(size_t)-1)
+#define ONES ((size_t)-1/UCHAR_MAX)
+#define HIGHS (ONES * (UCHAR_MAX/2+1))
+#define HASZERO(x) (((x)-ONES) & ~(x) & HIGHS)
+
+void *musl_memccpy(void *restrict dest, const void *restrict src, int c, size_t n)
+{
+	unsigned char *d = dest;
+	const unsigned char *s = src;
+
+	c = (unsigned char)c;
+#ifdef __GNUC__
+	typedef size_t __attribute__((__may_alias__)) word;
+	word *wd;
+	const word *ws;
+	if (((uintptr_t)s & ALIGN) == ((uintptr_t)d & ALIGN)) {
+		for (; ((uintptr_t)s & ALIGN) && n && (*d=*s)!=c; n--, s++, d++);
+		if ((uintptr_t)s & ALIGN) goto tail;
+		size_t k = ONES * c;
+		wd=(void *)d; ws=(const void *)s;
+		for (; n>=sizeof(size_t) && !HASZERO(*ws^k);
+		       n-=sizeof(size_t), ws++, wd++) *wd = *ws;
+		d=(void *)wd; s=(const void *)ws;
+	}
+#endif
+	for (; n && (*d=*s)!=c; n--, s++, d++);
+tail:
+	if (n) return d+1;
+	return 0;
+}
diff --git a/musl.h b/musl.h
index ff07821..4349622 100644
--- a/musl.h
+++ b/musl.h
@@ -3,6 +3,7 @@
 #include <stddef.h>
 
 void *musl_memcpy(void *dst, const void *src, size_t len);
+void *musl_memccpy(void *restrict dest, const void *restrict src, int c, size_t n);
 void *musl_memmove(void *dst, const void *src, size_t len);
 void *musl_memset(void *dst, int value, size_t len);
 wchar_t *musl_wmemset(wchar_t *dst, wchar_t value, size_t len);
diff --git a/test/.gitignore b/test/.gitignore
index cf4e256..45cabd2 100644
--- a/test/.gitignore
+++ b/test/.gitignore
@@ -45,6 +45,10 @@ memcpy_buffer_overflow
 memcpy_read_overflow
 memcpy_valid_same
 memcpy_valid_mismatched
+memccpy_buffer_overflow
+memccpy_read_overflow
+memccpy_valid_same
+memccpy_valid_mismatched
 memmove_buffer_overflow
 memmove_read_overflow
 memmove_valid_same
diff --git a/test/Makefile b/test/Makefile
index 8d29a9c..76f86f0 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -72,6 +72,10 @@ EXECUTABLES := \
     memcpy_read_overflow \
     memcpy_valid_same \
     memcpy_valid_mismatched \
+    memccpy_buffer_overflow \
+    memccpy_read_overflow \
+    memccpy_valid_same \
+    memccpy_valid_mismatched \
     memmove_buffer_overflow \
     memmove_read_overflow \
     memmove_valid_same \
diff --git a/test/memccpy_buffer_overflow.c b/test/memccpy_buffer_overflow.c
new file mode 100644
index 0000000..ca0c5d1
--- /dev/null
+++ b/test/memccpy_buffer_overflow.c
@@ -0,0 +1,15 @@
+#include <stdlib.h>
+#include <string.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    char *firstbuffer = malloc(16);
+    char *secondbuffer = malloc(32);
+    if (!firstbuffer && !secondbuffer) {
+        return 1;
+    }
+    memset(secondbuffer, 'a', 32);
+    memccpy(firstbuffer, secondbuffer, 'b', 32);
+    return 1;
+}
diff --git a/test/memccpy_read_overflow.c b/test/memccpy_read_overflow.c
new file mode 100644
index 0000000..3b15f53
--- /dev/null
+++ b/test/memccpy_read_overflow.c
@@ -0,0 +1,15 @@
+#include <stdlib.h>
+#include <string.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    char *firstbuffer = malloc(32);
+    char *secondbuffer = malloc(16);
+    if (!firstbuffer && !secondbuffer) {
+        return 1;
+    }
+    memset(secondbuffer, 'a', 16);
+    memccpy(firstbuffer, secondbuffer, 'b', 32);
+    return 1;
+}
diff --git a/test/memccpy_valid_mismatched.c b/test/memccpy_valid_mismatched.c
new file mode 100644
index 0000000..b5434f7
--- /dev/null
+++ b/test/memccpy_valid_mismatched.c
@@ -0,0 +1,15 @@
+#include <stdlib.h>
+#include <string.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    char *firstbuffer = malloc(32);
+    char *secondbuffer = malloc(16);
+    if (!firstbuffer && !secondbuffer) {
+        return 1;
+    }
+    memset(secondbuffer, 'a', 16);
+    memccpy(firstbuffer, secondbuffer, 'b', 16);
+    return 0;
+}
diff --git a/test/memccpy_valid_same.c b/test/memccpy_valid_same.c
new file mode 100644
index 0000000..a9ba59b
--- /dev/null
+++ b/test/memccpy_valid_same.c
@@ -0,0 +1,15 @@
+#include <stdlib.h>
+#include <string.h>
+
+#include "test_util.h"
+
+OPTNONE int main(void) {
+    char *firstbuffer = malloc(16);
+    char *secondbuffer = malloc(16);
+    if (!firstbuffer && !secondbuffer) {
+        return 1;
+    }
+    memset(secondbuffer, 'a', 16);
+    memccpy(firstbuffer, secondbuffer, 'b', 16);
+    return 0;
+}
diff --git a/test/test_smc.py b/test/test_smc.py
index 3a4c6f8..13a4d8d 100644
--- a/test/test_smc.py
+++ b/test/test_smc.py
@@ -262,6 +262,30 @@ class TestSimpleMemoryCorruption(unittest.TestCase):
             "memcpy_valid_mismatched")
         self.assertEqual(returncode, 0)
 
+    def test_memccpy_buffer_overflow(self):
+        _stdout, stderr, returncode = self.run_test(
+            "memccpy_buffer_overflow")
+        self.assertEqual(returncode, -6)
+        self.assertEqual(stderr.decode(
+            "utf-8"), "fatal allocator error: memccpy buffer overflow\n")
+
+    def test_memccpy_read_overflow(self):
+        _stdout, stderr, returncode = self.run_test(
+            "memccpy_read_overflow")
+        self.assertEqual(returncode, -6)
+        self.assertEqual(stderr.decode(
+            "utf-8"), "fatal allocator error: memccpy read overflow\n")
+
+    def test_memccpy_valid_same(self):
+        _stdout, _stderr, returncode = self.run_test(
+            "memccpy_valid_same")
+        self.assertEqual(returncode, 0)
+
+    def test_memccpy_valid_mismatched(self):
+        _stdout, _stderr, returncode = self.run_test(
+            "memccpy_valid_mismatched")
+        self.assertEqual(returncode, 0)
+
     def test_memmove_buffer_overflow(self):
         _stdout, stderr, returncode = self.run_test(
             "memmove_buffer_overflow")
-- 
2.49.0

Also there are three calls to memcpy() in ./random.c which might need to be converted to h_memcpy_internal().

@SkewedZeppelin SkewedZeppelin force-pushed the memcpy-sanity branch 2 times, most recently from 0beae2b to 0cada13 Compare March 30, 2025 14:11
@SkewedZeppelin
Copy link
Author

SkewedZeppelin commented Mar 30, 2025

I'm seeing a memccpy read overflow on both lvm immediately on start and gdm when trying to login

here is lvm, it looks correct

src size: 250, length: 256
fatal allocator error: memccpy read overflow

#4  0x00007ffff7f97113 in fatal_error (s=s@entry=0x7ffff7f9e629 "memccpy read overflow")
    at /home/tad/Development/CPP/rpm-hardened_malloc/hardened_malloc-2025012700-build/hardened_malloc/util.c:42
        prefix = 0x7ffff7f9e420 "fatal allocator error: "
#5  0x00007ffff7f9c729 in memccpy(void * __restrict__, const void * __restrict__, int, size_t) (
    dst=dst@entry=0x7fffffffc200, src=src@entry=0x7ea3bc2c23fe, value=value@entry=0, len=len@entry=256)
    at /home/tad/Development/CPP/rpm-hardened_malloc/hardened_malloc-2025012700-build/hardened_malloc/h_malloc.c:1912
No locals.
#6  0x0000555555557a83 in dm_strncpy (dest=0x7fffffffc200 "String", src=<optimized out>, n=256)
    at ../device_mapper/libdm-string.c:438
No locals.
#7  _dm_strncpy (dest=0x7fffffffc200 "String", src=<optimized out>, n=256) at ../lib/misc/util.h:42
No locals.
#8  _val_str_to_num (str=str@entry=0x7ea3bc2c23fe "String")
    at /usr/src/debug/lvm2-2.03.30-3.fc42.x86_64/tools/command.c:210
        name = "String\000de", '\000' <repeats 240 times>...
        new = <optimized out>
        i = <optimized out>

edit: it seems they're wholly reliant on c being found before n is reached

we could augment musl_memccpy to take the src size and abort there if reached

@cgzones
Copy link
Contributor

cgzones commented Apr 5, 2025

I'm seeing a memccpy read overflow on both lvm immediately on start and gdm when trying to login

lvm2 uses it as strncpy implementation, https://sources.debian.org/src/lvm2/2.03.31-1/libdm/libdm-string.c/?hl=438#L438:

int dm_strncpy(char *dest, const char *src, size_t n)
{
	if (memccpy(dest, src, 0, n))
		return 1;

	if (n > 0)
		dest[n - 1] = '\0';

	return 0;
}

A targeted fix could be to not check the source length if the stop character is 0. This will still avoid out-of-buffer-writes (just no out-of-buffer-reads).

The function bcopy(3) and swab(3) could also be valid targets:

patch
diff --git a/Makefile b/Makefile
index d18d0b3..0863bbc 100644
--- a/Makefile
+++ b/Makefile
@@ -41,7 +41,7 @@ LDFLAGS := $(LDFLAGS) -Wl,-O1,--as-needed,-z,defs,-z,relro,-z,now,-z,nodlopen,-z
 
 SOURCES := chacha.c h_malloc.c memory.c pages.c random.c util.c
 ifeq ($(CONFIG_BLOCK_OPS_CHECK_SIZE),true)
-    SOURCES += memcpy.c memccpy.c memmove.c memset.c wmemset.c
+    SOURCES += memcpy.c memccpy.c memmove.c memset.c swab.c wmemset.c
     BOSC_EXTRAS := musl.h
 endif
 OBJECTS := $(SOURCES:.c=.o)
@@ -148,6 +148,8 @@ $(OUT)/memmove.o: memmove.c musl.h $(CONFIG_FILE) | $(OUT)
        $(COMPILE.c) -Wno-cast-align $(OUTPUT_OPTION) $<
 $(OUT)/memset.o: memset.c musl.h $(CONFIG_FILE) | $(OUT)
        $(COMPILE.c) -Wno-cast-align $(OUTPUT_OPTION) $<
+$(OUT)/swab.o: swab.c musl.h $(CONFIG_FILE) | $(OUT)
+       $(COMPILE.c) -Wno-cast-align $(OUTPUT_OPTION) $<
 $(OUT)/wmemset.o: wmemset.c musl.h $(CONFIG_FILE) | $(OUT)
        $(COMPILE.c) $(OUTPUT_OPTION) $<
 
diff --git a/h_malloc.c b/h_malloc.c
index c99c0ca..fbc2c13 100644
--- a/h_malloc.c
+++ b/h_malloc.c
@@ -1940,6 +1940,27 @@ EXPORT void *memset(void *dst, int value, size_t len) {
     return musl_memset(dst, value, len);
 }
 
+EXPORT void bcopy(const void *src, void *dst, size_t len) {
+    memmove(dst, src, len);
+}
+
+EXPORT void swab(const void *restrict src, void *restrict dst, ssize_t len) {
+    if (unlikely(len <= 0)) {
+        return;
+    }
+    size_t length = len;
+    if (unlikely(dst < (src + length) && (dst + length) > src)) {
+        fatal_error("swab overlap");
+    }
+    if (unlikely(length > malloc_object_size(src))) {
+        fatal_error("swab read overflow");
+    }
+    if (unlikely(length > malloc_object_size(dst))) {
+        fatal_error("swab buffer overflow");
+    }
+    return musl_swab(src, dst, len);
+}
+
 EXPORT wchar_t *wmemcpy(wchar_t *restrict dst, const wchar_t *restrict src, size_t len) {
     if (unlikely(dst == src || len == 0)) {
         return dst;
diff --git a/include/h_malloc.h b/include/h_malloc.h
index 7974ff5..49ab36b 100644
--- a/include/h_malloc.h
+++ b/include/h_malloc.h
@@ -60,6 +60,8 @@ void *memcpy(void *dst, const void *src, size_t len);
 void *memccpy(void *dst, const void *src, int value, size_t len);
 void *memmove(void *dst, const void *src, size_t len);
 void *memset(void *dst, int value, size_t len);
+void bcopy(const void *src, void *dst, size_t len);
+void swab(const void *src, void *dst, ssize_t len);
 wchar_t *wmemcpy(wchar_t *dst, const wchar_t *src, size_t len);
 wchar_t *wmemmove(wchar_t *dst, const wchar_t *src, size_t len);
 wchar_t *wmemset(wchar_t *dst, wchar_t value, size_t len);
diff --git a/musl.h b/musl.h
index 4349622..de75eb7 100644
--- a/musl.h
+++ b/musl.h
@@ -1,9 +1,11 @@
 #pragma once
 
 #include <stddef.h>
+#include <sys/types.h>
 
 void *musl_memcpy(void *dst, const void *src, size_t len);
 void *musl_memccpy(void *restrict dest, const void *restrict src, int c, size_t n);
 void *musl_memmove(void *dst, const void *src, size_t len);
 void *musl_memset(void *dst, int value, size_t len);
+void musl_swab(const void *_src, void *_dest, ssize_t n);
 wchar_t *musl_wmemset(wchar_t *dst, wchar_t value, size_t len);
diff --git a/swab.c b/swab.c
new file mode 100644
index 0000000..29cdb2f
--- /dev/null
+++ b/swab.c
@@ -0,0 +1,17 @@
+#include "musl.h"
+
+/* Copied from musl libc version 1.2.5 licensed under the MIT license */
+
+#include <unistd.h>
+
+void musl_swab(const void *restrict _src, void *restrict _dest, ssize_t n)
+{
+       const char *src = _src;
+       char *dest = _dest;
+       for (; n>1; n-=2) {
+               dest[0] = src[1];
+               dest[1] = src[0];
+               dest += 2;
+               src += 2;
+       }
+}

Also mempcpy(3) and wmempcpy(3) are trivial to cover:

patch
diff --git a/h_malloc.c b/h_malloc.c
index dbdf01f..ff174a0 100644
--- a/h_malloc.c
+++ b/h_malloc.c
@@ -1946,6 +1946,10 @@ EXPORT void *memmove(void *dst, const void *src, size_t len) {
     return musl_memmove(dst, src, len);
 }
 
+EXPORT void *mempcpy(void *restrict dst, const void *restrict src, size_t len) {
+    return memcpy(dst, src, len) + len;
+}
+
 EXPORT void *memset(void *dst, int value, size_t len) {
     if (unlikely(len == 0)) {
         return dst;
@@ -2008,6 +2012,10 @@ EXPORT wchar_t *wmemmove(wchar_t *dst, const wchar_t *src, size_t len) {
     return (wchar_t *)musl_memmove((char *)dst, (const char *)src, lenAdj);
 }
 
+EXPORT wchar_t *wmempcpy(wchar_t *restrict dst, const wchar_t *restrict src, size_t len) {
+    return wmemcpy(dst, src, len) + len;
+}
+
 EXPORT wchar_t *wmemset(wchar_t *dst, wchar_t value, size_t len) {
     if (unlikely(len == 0)) {
         return dst;
diff --git a/include/h_malloc.h b/include/h_malloc.h
index b31f62e..798ebd2 100644
--- a/include/h_malloc.h
+++ b/include/h_malloc.h
@@ -61,11 +61,13 @@ void h_free(void *ptr);
 void *memcpy(void *dst, const void *src, size_t len);
 void *memccpy(void *dst, const void *src, int value, size_t len);
 void *memmove(void *dst, const void *src, size_t len);
+void *mempcpy(void *dst, const void *src, size_t len);
 void *memset(void *dst, int value, size_t len);
 void bcopy(const void *src, void *dst, size_t len);
 void swab(const void *src, void *dst, ssize_t len);
 wchar_t *wmemcpy(wchar_t *dst, const wchar_t *src, size_t len);
 wchar_t *wmemmove(wchar_t *dst, const wchar_t *src, size_t len);
+wchar_t *wmempcpy(wchar_t *dst, const wchar_t *src, size_t len);
 wchar_t *wmemset(wchar_t *dst, wchar_t value, size_t len);
 #define h_memcpy_internal musl_memcpy
 #define h_memmove_internal musl_memmove

Signed-off-by: Tavi <[email protected]>
Co-authored-by: =?UTF-8?q?Christian=20G=C3=B6ttsche?= <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants