Repository navigation
[HUST CSE][dfs] fix stack buffer overflow on long path components in devtmpfs - #11842
HANYAODONG wants to merge 3 commits into
Conversation
…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/<long name>", O_CREAT) - including from user mode through
sys_open() - and from the msh command mkdir /dev/<long name>.
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]
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-10-17 21:58 CST)
📝 Review Instructions
|
|
|
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
Review / 审查
结论 / Conclusion: no blocking issue found; the bounds-checking fix is directionally correct.
The new parent_size/file_size/name_size checks prevent the reported stack overwrite, and the vnode cleanup path is present. The change is small and keeps devtmpfs consistent with tmpfs.
建议 / Suggestions
-
components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c:63still dereferencespath[0]before validatingpath. This is probably protected by the VFS dentry invariant, but checkingpath == RT_NULLbefore the assertion would make this helper robust against malformed internal callers and consistent with the tmpfs implementation.if (path == RT_NULL || parent_path == RT_NULL || file_name == RT_NULL ...) return -EINVAL; RT_ASSERT(path[0] == '/');
-
Please add boundary regression tests for component lengths 254, 255, and 256 bytes, covering both vnode creation and lookup. The test should verify that an overlong component cannot modify neighboring stack data and that the expected error is observable by the caller.
The PR description already contains a useful reproduction and validation record. / PR 描述中的复现和验证信息比较完整。
|
…ndary tests Address the review of RT-Thread#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]
|
Thanks for the review! Both suggestions are addressed in 6b5e2f7. 1. Validate before dereference - 2. Boundary tests - added Verified on
|
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.
|
Pushed a4163bf to fix the clang-format violations that the "Check File |
This branch has not been deployed
3 participants
[
为什么提交这份PR (why to submit this PR)
components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c中的_path_separate()与_get_subdir()没有接收缓冲区长度参数，会把任意长度的路径分量直接
rt_memcpy进调用者的固定大小栈数组：_path_separate()（:82-83）把最后一级分量拷进file_name[DIRENT_NAME_MAX]（256 字节），随后还在
file_name[path_q - path_p]写一个 NUL；_get_subdir()（:104）逐字节写subdir_name[DIRENT_NAME_MAX]（256 字节）。DIRENT_NAME_MAX = 256（components/libc/compilers/common/include/dirent.h:57），而
dentry->pathname最长可达DFS_PATH_MAX - 4 = 4092字节（
_dentry_create()只剥掉挂载点/dev前缀，dfs_dentry.c:74-85）。因此单个路径分量超过 255 字节时即发生栈缓冲区溢出，越界可达约 3.8 KB，
写入的内容与长度均由调用者控制，能够覆盖保存的返回地址。
这不是理论问题：上游
tmpfs的同名函数dfs_tmpfs.c:55-140已经加上了parent_size/file_size/name_size校验并返回-ENAMETOOLONG，只有 devfs 这一份漏了，属于同类修复的漏项。
你的解决方案是什么 (what is your solution)
让 devfs 与 tmpfs 的实现保持一致：
_path_separate()增加parent_size/file_size参数，越界时返回-ENAMETOOLONG；_get_subdir()增加name_size参数，越界时返回-ENAMETOOLONG；devtmpfs_create_vnode()失败时走原有的dfs_vnode_destroy()清理路径并返回
NULL，devtmpfs_file_lookup()失败时返回NULL。改动 1 file changed, 48 insertions(+), 8 deletions(-)，不改变正常路径行为。
请提供验证的bsp和config (provide the config and bsp)
BSP: bsp/qemu-vexpress-a9
.config: 使用该 BSP 默认配置，未额外改动；相关项为
CONFIG_RT_USING_DFS=y
CONFIG_RT_USING_DFS_V2=y
CONFIG_RT_USING_DFS_DEVFS=y
action: 尚未触发 fork 上的 action；本地已完成等价的编译验证，见下方"验证情况"。
如需 CI 链接，可在本 PR 建立后补充。
验证情况：
使用 arm-none-eabi-gcc 13.3.0 在 bsp/qemu-vexpress-a9 上编译链接通过
（text 552603, data 34384, bss 79600）；
在 QEMU (vexpress-a9) 上端到端复现，触发方式为
open("/dev/<300 x 'A'>", O_CREAT|O_RDWR, 0666)：修复前：
backtrace: ... 41414141 <- 'A' 覆盖了返回地址
data abort: Exception: pc :0x60011718
用 arm-none-eabi-addr2line 解析该 pc，得到
devtmpfs_file_lookup components/dfs/dfs_v2/filesystems/devfs/devtmpfs.c:220
即
_path_separate()的越界写破坏了devtmpfs_create_vnode()的栈帧。修复后，同一输入：
open() = -1, errno = ENOENT
RETURNED NORMALLY -- kernel stack survived
调用链：
open("/dev/<超长分量>", O_CREAT|O_RDWR, 0666)→
dfs_file_open()（dfs_file.c:565）→dfs_dentry_lookup()（:596）查找失败→
O_CREAT分支 →dfs_dentry_create()（:668）→devtmpfs_create_vnode()（:487）→
_path_separate():82 越界；第二条入口是dfs_file_mknod()（:903）。Shell 等价操作：
mkdir /dev/<超长名>。]
当前拉取/合并请求的状态 Intent for your PR
必须选择一项 Choose one (Mandatory):
代码质量 Code Quality：
我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:
#if 0代码，不包含已经被注释了的代码 All redundant code is removed and cleaned up