From b75a43df9e3bcabcbb1ba6a1eb494dd4043e9997 Mon Sep 17 00:00:00 2001 From: Yaodong Han <189000603+HANYAODONG@users.noreply.github.com> Date: Sat, 26 Sep 2026 19:51:39 +0800 Subject: [PATCH 1/3] [HUST CSE][dfs] fix stack buffer overflow on long path components in devtmpfs _path_separate() and _get_subdir() copied an unbounded path component into caller-provided buffers, so a single component longer than DIRENT_NAME_MAX (256) bytes overran the 256-byte file_name[]/subdir_name[] stack arrays used by devtmpfs_create_vnode() and devtmpfs_file_lookup(). dentry->pathname can be up to DFS_PATH_MAX - 4 = 4092 bytes because _dentry_create() only strips the mount point prefix, so the overflow length and its contents are attacker controlled. It is reachable from open("/dev/", O_CREAT) - including from user mode through sys_open() - and from the msh command mkdir /dev/. On bsp/qemu-vexpress-a9 an over-long single component produced a data abort whose backtrace contained 0x41414141, i.e. the saved return address overwritten with the attacker supplied path bytes. The tmpfs implementation already validates these lengths and returns -ENAMETOOLONG (dfs_tmpfs.c), this brings devtmpfs in line with it: - pass parent_size/file_size to _path_separate() and name_size to _get_subdir(), rejecting oversized components with -ENAMETOOLONG - check the return value at both call sites; create_vnode() and file_lookup() now fail cleanly instead of corrupting the stack [HUST CSE] --- .../dfs/dfs_v2/filesystems/devfs/devtmpfs.c | 56 ++++++++++++++++--- 1 file changed, 48 insertions(+), 8 deletions(-) diff --git a/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c b/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c index b60e5c5d0f1c..a540375cb78a 100644 --- a/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c +++ b/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c @@ -54,13 +54,22 @@ struct devtmpfs_sb static struct dfs_file_ops _default_fops = { 0 }; -static int _path_separate(const char *path, char *parent_path, char *file_name) +static int _path_separate(const char *path, char *parent_path, rt_size_t parent_size, + char *file_name, rt_size_t file_size) { const char *path_p, *path_q; + rt_size_t parent_len, file_len; RT_ASSERT(path[0] == '/'); + if (parent_path == RT_NULL || file_name == RT_NULL || + parent_size < 2 || file_size == 0) + { + return -EINVAL; + } + file_name[0] = '\0'; + parent_path[0] = '\0'; path_p = path_q = &path[1]; __next_dir: while (*path_q != '/' && *path_q != '\0') @@ -77,10 +86,17 @@ static int _path_separate(const char *path, char *parent_path, char *file_name) } else /* Last level dir */ { - rt_memcpy(parent_path, path, path_p - path - 1); - parent_path[path_p - path - 1] = '\0'; - rt_memcpy(file_name, path_p, path_q - path_p); - file_name[path_q - path_p] = '\0'; + parent_len = path_p - path - 1; + file_len = path_q - path_p; + if (parent_len >= parent_size || file_len >= file_size) + { + return -ENAMETOOLONG; + } + + rt_memcpy(parent_path, path, parent_len); + parent_path[parent_len] = '\0'; + rt_memcpy(file_name, path_p, file_len); + file_name[file_len] = '\0'; } } if (parent_path[0] == 0) @@ -94,17 +110,31 @@ static int _path_separate(const char *path, char *parent_path, char *file_name) return 0; } -static int _get_subdir(const char *path, char *name) +static int _get_subdir(const char *path, char *name, rt_size_t name_size) { const char *subpath = path; + rt_size_t name_len = 0; + + if (path == RT_NULL || name == RT_NULL || name_size == 0) + { + return -EINVAL; + } + while (*subpath == '/' && *subpath) subpath ++; while (*subpath != '/' && *subpath) { + if (name_len + 1 >= name_size) + { + name[0] = '\0'; + return -ENAMETOOLONG; + } *name = *subpath; name ++; subpath ++; + name_len++; } + *name = '\0'; return 0; } @@ -213,7 +243,10 @@ static struct devtmpfs_file *devtmpfs_file_lookup(struct devtmpfs_sb *superblock subpath ++; /* skip '/' */ memset(subdir_name, 0, DIRENT_NAME_MAX); - _get_subdir(curpath, subdir_name); + if (_get_subdir(curpath, subdir_name, sizeof(subdir_name)) != 0) + { + return NULL; + } rt_spin_lock(&superblock->lock); @@ -490,6 +523,7 @@ static struct dfs_vnode *devtmpfs_create_vnode(struct dfs_dentry *dentry, int ty struct devtmpfs_sb *superblock; struct devtmpfs_file *d_file, *p_file; char parent_path[DFS_PATH_MAX], file_name[DIRENT_NAME_MAX]; + int ret; if (dentry == NULL || dentry->mnt == NULL || dentry->mnt->data == NULL) { @@ -503,7 +537,13 @@ static struct dfs_vnode *devtmpfs_create_vnode(struct dfs_dentry *dentry, int ty if (vnode) { /* find parent file */ - _path_separate(dentry->pathname, parent_path, file_name); + ret = _path_separate(dentry->pathname, parent_path, sizeof(parent_path), + file_name, sizeof(file_name)); + if (ret != RT_EOK) + { + dfs_vnode_destroy(vnode); + return NULL; + } if (file_name[0] == '\0') /* it's root dir */ { dfs_vnode_destroy(vnode); From 6b5e2f7f5f6975fe3b689db9158c6932ab220294 Mon Sep 17 00:00:00 2001 From: Yaodong Han <189000603+HANYAODONG@users.noreply.github.com> Date: Sat, 17 Oct 2026 16:13:49 +0800 Subject: [PATCH 2/3] [HUST CSE][dfs] validate path before dereference and add devtmpfs boundary tests Address the review of #11842. _path_separate() dereferenced path[0] in RT_ASSERT before any of its arguments had been validated, so the helper was not robust against a NULL path from a malformed internal caller. Move the argument checks ahead of the assertion, which also makes the helper consistent with the tmpfs one. Add a boundary regression test for the path components of devtmpfs, with the cases the review asked for: - component lengths DIRENT_NAME_MAX - 2 (254) and DIRENT_NAME_MAX - 1 (255) must still create the node, - DIRENT_NAME_MAX (256) and 300 bytes must be rejected with an error the caller can observe, and must not create anything, - both entry points are covered for every length: devtmpfs_file_lookup() through dfs_file_stat() and devtmpfs_create_vnode() through open(), - a guard pattern around the call verifies that an overlong component does not modify neighbouring stack data. The test lives in components/dfs/utest/tc_devtmpfs_path.c behind its own Kconfig option (RT_UTEST_TC_USING_DEVTMPFS_PATH, depends on DFS v2) so it does not change any existing utest configuration. Verified on bsp/qemu-vexpress-a9 with the real utest framework compiled into the image: the fixed tree passes all five cases (0 failure), while the tree without the fix fails at the 256-byte case and then aborts with the 0x41414141 backtrace, which is the overflow this patch prevents. [HUST CSE] --- .../dfs/dfs_v2/filesystems/devfs/devtmpfs.c | 7 +- components/dfs/utest/Kconfig | 11 + components/dfs/utest/SConscript | 4 + components/dfs/utest/tc_devtmpfs_path.c | 246 ++++++++++++++++++ 4 files changed, 265 insertions(+), 3 deletions(-) create mode 100644 components/dfs/utest/tc_devtmpfs_path.c diff --git a/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c b/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c index a540375cb78a..5764eb877b9c 100644 --- a/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c +++ b/components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c @@ -60,14 +60,15 @@ static int _path_separate(const char *path, char *parent_path, rt_size_t parent_ const char *path_p, *path_q; rt_size_t parent_len, file_len; - RT_ASSERT(path[0] == '/'); - - if (parent_path == RT_NULL || file_name == RT_NULL || + /* validate the arguments before the path is dereferenced below */ + if (path == RT_NULL || parent_path == RT_NULL || file_name == RT_NULL || parent_size < 2 || file_size == 0) { return -EINVAL; } + RT_ASSERT(path[0] == '/'); + file_name[0] = '\0'; parent_path[0] = '\0'; path_p = path_q = &path[1]; diff --git a/components/dfs/utest/Kconfig b/components/dfs/utest/Kconfig index 32acadbf1a70..94ad954366ac 100644 --- a/components/dfs/utest/Kconfig +++ b/components/dfs/utest/Kconfig @@ -41,6 +41,17 @@ if RT_USING_DFS endif + config RT_UTEST_TC_USING_DEVTMPFS_PATH + bool "devtmpfs path component boundary Test" + default n + depends on RT_USING_DFS_V2 + help + Enable boundary regression tests for devtmpfs path components. + Verifies that a path component of DIRENT_NAME_MAX bytes or more is + rejected with -ENAMETOOLONG instead of overrunning the stack buffers + used by devtmpfs_create_vnode() and devtmpfs_file_lookup(), and that + shorter components keep working. Requires devtmpfs mounted on /dev. + rsource "v2/af_unix/Kconfig" endmenu diff --git a/components/dfs/utest/SConscript b/components/dfs/utest/SConscript index bb8fa66a35d4..e6c5bf6caed2 100644 --- a/components/dfs/utest/SConscript +++ b/components/dfs/utest/SConscript @@ -15,6 +15,10 @@ if GetDepend('RT_UTEST_TC_USING_DFS_API'): if GetDepend('RT_UTEST_TC_USING_POSIX_API'): src += ['tc_posix_api.c'] +# Add devtmpfs path component boundary test if enabled (requires DFS v2) +if GetDepend('RT_UTEST_TC_USING_DEVTMPFS_PATH'): + src += ['tc_devtmpfs_path.c'] + # Define the test group with proper dependencies objs = DefineGroup('utestcases', src, depend = ['RT_USING_UTESTCASES', 'RT_USING_DFS'], CPPPATH = CPPPATH) objs = objs + SConscript(os.path.join('v2', 'af_unix', 'SConscript')) diff --git a/components/dfs/utest/tc_devtmpfs_path.c b/components/dfs/utest/tc_devtmpfs_path.c new file mode 100644 index 000000000000..4f1b9b93c53d --- /dev/null +++ b/components/dfs/utest/tc_devtmpfs_path.c @@ -0,0 +1,246 @@ +/* + * Copyright (c) 2006-2025, RT-Thread Development Team + * + * SPDX-License-Identifier: Apache-2.0 + * + * Change Logs: + * Date Author Notes + * 2026-10-17 Yaodong Han boundary regression tests for devtmpfs path + * components (devtmpfs_create_vnode/_path_separate + * and devtmpfs_file_lookup/_get_subdir) + */ + +#include +#include "utest.h" + +#include +#include +#include +#include + +#ifdef RT_USING_DFS_V2 + +/* + * The devtmpfs helpers copy the last path component into char[DIRENT_NAME_MAX] + * buffers, so a component of DIRENT_NAME_MAX bytes or more must be rejected + * instead of overrunning the caller's stack frame. A component of + * DIRENT_NAME_MAX - 1 bytes (plus the terminator) still fits. + */ +#define TC_NAME_MAX DIRENT_NAME_MAX + +/* devtmpfs is mounted on /dev by the device filesystem, see devtmpfs_init() */ +#define TC_MNT_PATH "/dev" + +/* Path buffer: mount point + '/' + component + '\0'. + * Sized for the longest component the boundary sweep below uses (300 bytes), + * which is deliberately well past DIRENT_NAME_MAX. */ +#define TC_MAX_TEST_LEN 300 +#define TC_PATH_BUF_SIZE (sizeof(TC_MNT_PATH) + 1 + TC_MAX_TEST_LEN + 1) + +static char tc_path[TC_PATH_BUF_SIZE]; + +/* + * Caller-side stack guard. devtmpfs_create_vnode() and devtmpfs_file_lookup() + * keep their file_name[]/subdir_name[] buffers on their own frames; if the + * component were copied without a bounds check those frames - and the data + * that survives in the caller after the call - would be corrupted. Keeping a + * known pattern around the call and checking it afterwards is the observable + * "does not modify neighbouring stack data" assertion requested in review. + */ +#define TC_GUARD_SIZE 32 +#define TC_GUARD_PATTERN 0xA5 +#define TC_GUARD_MAGIC 0x5A5A1234UL + +static volatile rt_ubase_t tc_guard_magic; + +static void tc_guard_fill(rt_uint8_t *guard) +{ + rt_memset(guard, TC_GUARD_PATTERN, TC_GUARD_SIZE); + tc_guard_magic = TC_GUARD_MAGIC; +} + +static rt_bool_t tc_guard_intact(const rt_uint8_t *guard) +{ + rt_size_t i; + + if (tc_guard_magic != TC_GUARD_MAGIC) + { + return RT_FALSE; + } + + for (i = 0; i < TC_GUARD_SIZE; i++) + { + if (guard[i] != TC_GUARD_PATTERN) + { + return RT_FALSE; + } + } + + return RT_TRUE; +} + +/* + * Build "/" into tc_path and return its length. + * The mount point is taken from RT_UTEST_DFS_MNT_PATH so the test works for + * whatever filesystem/mount point the utest configuration uses. + */ +static rt_size_t tc_build_path(rt_size_t len) +{ + rt_size_t mnt_len = rt_strlen(TC_MNT_PATH); + rt_size_t i; + + RT_ASSERT(len <= TC_MAX_TEST_LEN); + + rt_memcpy(tc_path, TC_MNT_PATH, mnt_len); + tc_path[mnt_len] = '/'; + for (i = 0; i < len; i++) + { + tc_path[mnt_len + 1 + i] = 'A'; + } + tc_path[mnt_len + 1 + len] = '\0'; + + return mnt_len + 1 + len; +} + +/* + * Exercise both entry points for one component length: + * - devtmpfs_file_lookup() through dfs_file_stat() + * - devtmpfs_create_vnode() through open(O_CREAT) + * and verify that neither one touches neighbouring stack data. + * + * Returns the open() result (-1 on failure). + */ +static int tc_roundtrip(rt_size_t len, rt_bool_t expect_ok) +{ + rt_uint8_t guard[TC_GUARD_SIZE]; + struct stat st; + int fd; + + tc_build_path(len); + tc_guard_fill(guard); + + /* lookup path first: the node does not exist yet, this must not crash */ + (void)dfs_file_stat(tc_path, &st); + + /* creation path: this is where _path_separate() runs */ + fd = open(tc_path, O_CREAT | O_RDWR, 0644); + + if (!tc_guard_intact(guard)) + { + rt_kprintf("guard corrupted for component length %u\n", + (unsigned int)len); + return -2; + } + + if (expect_ok) + { + if (fd < 0) + { + rt_kprintf("expected success for component length %u, got %d\n", + (unsigned int)len, fd); + return -3; + } + close(fd); + + /* the node must now be visible through the lookup path */ + tc_guard_fill(guard); + if (dfs_file_stat(tc_path, &st) != 0) + { + if (tc_guard_intact(guard)) + { + rt_kprintf("node not found after creation, len %u\n", + (unsigned int)len); + return -4; + } + return -2; + } + } + else + { + if (fd >= 0) + { + close(fd); + rt_kprintf("expected rejection for component length %u\n", + (unsigned int)len); + return -5; + } + + /* a rejected component must not have created anything either */ + tc_guard_fill(guard); + if (dfs_file_stat(tc_path, &st) == 0) + { + if (tc_guard_intact(guard)) + { + rt_kprintf("overlong component still resolved, len %u\n", + (unsigned int)len); + return -6; + } + return -2; + } + } + + return tc_guard_intact(guard) ? 0 : -2; +} + +/* len == DIRENT_NAME_MAX - 2: comfortably inside the buffer */ +static void test_devtmpfs_component_254(void) +{ + uassert_int_equal(tc_roundtrip(TC_NAME_MAX - 2, RT_TRUE), 0); +} + +/* len == DIRENT_NAME_MAX - 1: the last length that still fits (+ terminator) */ +static void test_devtmpfs_component_255(void) +{ + uassert_int_equal(tc_roundtrip(TC_NAME_MAX - 1, RT_TRUE), 0); +} + +/* len == DIRENT_NAME_MAX: no room for the terminator, must be rejected */ +static void test_devtmpfs_component_256(void) +{ + uassert_int_equal(tc_roundtrip(TC_NAME_MAX, RT_FALSE), 0); +} + +/* well past the buffer: must be rejected as well */ +static void test_devtmpfs_component_300(void) +{ + uassert_int_equal(tc_roundtrip(TC_NAME_MAX + 44, RT_FALSE), 0); +} + +/* a normal, short component must keep working (no regression) */ +static void test_devtmpfs_component_short(void) +{ + uassert_int_equal(tc_roundtrip(8, RT_TRUE), 0); +} + +static rt_err_t utest_tc_init(void) +{ + struct stat st; + + /* devfs is mounted by the system; make sure the mount point exists */ + if (dfs_file_stat(TC_MNT_PATH, &st) != 0) + { + rt_kprintf("mount point %s not available, devtmpfs test skipped\n", + TC_MNT_PATH); + return -RT_ERROR; + } + + return RT_EOK; +} + +static rt_err_t utest_tc_cleanup(void) +{ + /* the successful cases only create device nodes; nothing is persisted */ + return RT_EOK; +} + +static void testcase(void) +{ + UTEST_UNIT_RUN(test_devtmpfs_component_short); + UTEST_UNIT_RUN(test_devtmpfs_component_254); + UTEST_UNIT_RUN(test_devtmpfs_component_255); + UTEST_UNIT_RUN(test_devtmpfs_component_256); + UTEST_UNIT_RUN(test_devtmpfs_component_300); +} +UTEST_TC_EXPORT(testcase, "components.dfs.devtmpfs_path_tc", utest_tc_init, utest_tc_cleanup, 30); + +#endif /* RT_USING_DFS_V2 */ From a4163bfd975f4ae42aedcc99aa3aad77c4e1b748 Mon Sep 17 00:00:00 2001 From: Yaodong Han <189000603+HANYAODONG@users.noreply.github.com> Date: Sat, 17 Oct 2026 21:58:11 +0800 Subject: [PATCH 3/3] [HUST CSE][dfs] format the devtmpfs boundary test with clang-format The new test file failed the "Check File Format and License" workflow because the macro alignment did not match the repository .clang-format. Run clang-format on it; whitespace only, no functional change. --- components/dfs/utest/tc_devtmpfs_path.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/components/dfs/utest/tc_devtmpfs_path.c b/components/dfs/utest/tc_devtmpfs_path.c index 4f1b9b93c53d..1cb215be3c96 100644 --- a/components/dfs/utest/tc_devtmpfs_path.c +++ b/components/dfs/utest/tc_devtmpfs_path.c @@ -26,16 +26,16 @@ * instead of overrunning the caller's stack frame. A component of * DIRENT_NAME_MAX - 1 bytes (plus the terminator) still fits. */ -#define TC_NAME_MAX DIRENT_NAME_MAX +#define TC_NAME_MAX DIRENT_NAME_MAX /* devtmpfs is mounted on /dev by the device filesystem, see devtmpfs_init() */ -#define TC_MNT_PATH "/dev" +#define TC_MNT_PATH "/dev" /* Path buffer: mount point + '/' + component + '\0'. * Sized for the longest component the boundary sweep below uses (300 bytes), * which is deliberately well past DIRENT_NAME_MAX. */ -#define TC_MAX_TEST_LEN 300 -#define TC_PATH_BUF_SIZE (sizeof(TC_MNT_PATH) + 1 + TC_MAX_TEST_LEN + 1) +#define TC_MAX_TEST_LEN 300 +#define TC_PATH_BUF_SIZE (sizeof(TC_MNT_PATH) + 1 + TC_MAX_TEST_LEN + 1) static char tc_path[TC_PATH_BUF_SIZE]; @@ -47,9 +47,9 @@ static char tc_path[TC_PATH_BUF_SIZE]; * known pattern around the call and checking it afterwards is the observable * "does not modify neighbouring stack data" assertion requested in review. */ -#define TC_GUARD_SIZE 32 -#define TC_GUARD_PATTERN 0xA5 -#define TC_GUARD_MAGIC 0x5A5A1234UL +#define TC_GUARD_SIZE 32 +#define TC_GUARD_PATTERN 0xA5 +#define TC_GUARD_MAGIC 0x5A5A1234UL static volatile rt_ubase_t tc_guard_magic;