Skip to content

[dfs][cromfs] fix: bound the directory name read by getdents - #11911

Open
Yunlk wants to merge 3 commits into
RT-Thread:masterfrom
Yunlk:fix/cromfs-getdents-bound
Open

Yunlk wants to merge 3 commits into
RT-Thread:masterfrom
Yunlk:fix/cromfs-getdents-bound

Conversation

@Yunlk

@Yunlk Yunlk commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

拉取/合并请求描述:(PR description)

[

为什么提交这份PR (why to submit this PR)

关联 issue #11905(我已认领)。

dfs_cromfs_getdents() 直接用镜像里的 uint16 name_size 作为 memcpy 长度往 d_name 里拷,既不与 DIRENT_NAME_MAX 比较,也不校验名字是否落在刚加载的目录 blob 内:

d->d_namlen = sub_dirent->dirent.name_size;
d->d_reclen = (rt_uint16_t)sizeof(struct dirent);
memcpy(d->d_name, (char *)name, sub_dirent->dirent.name_size);
d->d_name[sub_dirent->dirent.name_size] = '\0';

d_name 只有 DIRENT_NAME_MAX(256) 字节,紧接着那行又把 NUL 写在 name_size 位置,所以 name_size 到 256 时原代码本身就已经越界一个字节;而 name_size 是镜像给的 uint16,最大可达 65535。

调用方也不做裁剪:msh 的 ls() 把 struct dirent 放在 shell 线程栈上,向 getdents 要 sizeof(struct dirent) 一个条目,于是超长目录名覆盖的是栈上对象,被覆盖的字节完全由镜像决定。issue 里的崩溃现场是 fp=0x42424242(即 'B'),ls 返回时跳到 0x42424246。

v1 与 v2 两份实现的这四行完全一样,都受影响。

你的解决方案是什么 (what is your solution)

在两份实现的循环里,先校验、再使用:

  1. 条目头自身必须落在已加载的 blob 内(file->pos + sizeof(cromfs_dirent) <= fi->size),否则 sub_dirent 的索引本身就越界;
  2. name_size < DIRENT_NAME_MAX(要留出结尾 NUL 的位置);
  3. file->pos + sizeof(cromfs_dirent) + name_size <= fi->size,保证名字不跑出 blob。

任一条不满足即 result = -EINVAL 跳出循环,函数返回 -EINVAL,不再执行拷贝。校验通过后改用一个已在范围内的局部量 entry_name_size 参与 memcpy、d_namlen 与 file->pos 推进,避免继续读镜像字段。

v1 用 file->pos,v2 用 file->fpos,其余逻辑一致。

请提供验证的bsp和config (provide the config and bsp)

  • BSP: bsp/qemu-vexpress-a9(DFS_V2,用于验证 v2 副本);v1 副本用同一套编译参数做语法检查(见下)
  • .config: 在 BSP 默认配置上加 CONFIG_RT_USING_DFS_CROMFS=y
  • action: 本地编译,未使用 GitHub Actions

验证情况:

项目 结果
v2 副本编译(vexpress-a9 / DFS_V2 / CROMFS) 通过
v1 副本语法检查 通过:用同一份编译参数(把 dfs_v2 路径换成 dfs_v1)跑 -fsyntax-only,与未修改的原始文件报出的错误完全相同(都只有 _crom_fops.lseek 一处类型不匹配,那是用 V2 的 rtconfig.h 编 V1 源码造成的环境错配,与本次改动无关)
运行时验证 已完成,见下

运行时验证用内存镜像绕开了 zlib 依赖:cromfs 支持通过 dfs_mount(..., "crom", ..., data) 的 data 指针直接读内存镜像(k230 / cvitek 就这么挂根文件系统),而目录遍历不涉及解压。本 PR 新增 utest components.dfs.v2.cromfs.dirent_name,在 RAM 里构造 256 字节头 + 一个目录项,挂载后调用 getdents。

同一个用例在本 PR 与 master 上的结果完全相反:

master 1c801efa(无修复)
  control image  -> ret=260, d_namlen=2, name="oo"
  oversized name -> ret=260, d_namlen=0        <- 512 字节被拷进 256 字节缓冲区
  ASSERT ((ret) not == (-22))
  prefetch abort:                              <- 溢出打崩调用方栈
  shutdown...

本 PR(有修复)
  control image  -> ret=260, d_namlen=2, name="oo"
  oversized name -> ret=-22, d_namlen=0        <- -EINVAL,调用方缓冲区未被写
  [  PASSED  ] testcase (components.dfs.v2.cromfs.dirent_name)

这个用例不需要块设备、不需要 zlib,可以直接进 CI。若维护者希望把它挂到 .github/utest/configs/ 的某个 cfg 里,请指出放在哪一组合适。
]

当前拉取/合并请求的状态 Intent for your PR

必须选择一项 Choose one (Mandatory):

  • 本拉取/合并请求是一个草稿版本 This PR is for a code-review and is intended to get feedback
  • 本拉取/合并请求是一个成熟版本 This PR is mature, and ready to be integrated into the repo

代码质量 Code Quality:

我在这个拉取/合并请求中已经考虑了 As part of this pull request, I've considered the following:

  • 已经仔细查看过代码改动的对比 Already check the difference between PR and old code
  • 代码风格正确,包括缩进空格,命名及其他风格 Style guide is adhered to, including spacing, naming and other styles
  • 没有垃圾代码,代码尽量精简,不包含#if 0代码,不包含已经被注释了的代码 All redundant code is removed and cleaned up
  • 所有变更均有原因及合理的,并且不会影响到其他软件组件代码或BSP All modifications are justified and not affect other components or BSP
  • 对难懂代码均提供对应的注释 I've commented appropriately where code is tricky
  • 代码是高质量的 Code in this PR is of high quality
  • 已经使用clang-format 源码格式化工具确保格式符合RT-Thread代码规范 This PR has been formatted with clang-format and complies with RT-Thread code specification
  • 如果是新增bsp, 已经添加ci检查到.github/ALL_BSP_COMPILE.json 详细请参考链接BSP自查

dfs_cromfs_getdents() used the directory entry's own name_size field from the
image as the length of a memcpy into d_name. It never compared that value
against DIRENT_NAME_MAX and never checked that the name lies inside the
directory blob it had just loaded.

d_name is DIRENT_NAME_MAX (256) bytes, and the line right after the copy
stores the terminating NUL at index name_size, so even the original code
already wrote one byte past the array whenever name_size reached 256.
name_size is a uint16 taken straight from the image, so it can be 65535.

Callers do not clip the result. msh's ls() keeps a single struct dirent on
the shell thread stack and asks getdents for sizeof(struct dirent), so a
crafted entry overflows that stack object with image-controlled bytes. The
reported crash ends with the saved frame pointer holding 0x42424242 and the
return from ls() transferring control to an attacker-chosen address.

The v1 and v2 copies are affected in the same way and both are fixed. An
entry whose name_size does not fit in d_name, or whose name does not fit in
the loaded blob, now makes getdents return -EINVAL instead of copying. The
entry header itself is bounds-checked against the blob as well, so the
sub_dirent index cannot run past the buffer either.

Refs RT-Thread#11905
@Yunlk
Yunlk requested a review from BernardXiong as a code owner October 21, 2026 06:04
@github-actions

Copy link
Copy Markdown

👋 感谢您对 RT-Thread 的贡献!Thank you for your contribution to RT-Thread!

为确保代码符合 RT-Thread 的编码规范,请在你的仓库中执行以下步骤运行代码格式化工作流(如果格式化CI运行失败)。
To ensure your code complies with RT-Thread's coding style, please run the code formatting workflow by following the steps below (If the formatting of CI fails to run).


🛠 操作步骤 | Steps

  1. 前往 Actions 页面 | Go to the Actions page
    点击进入工作流 → | Click to open workflow →

  2. 点击 Run workflow | Click Run workflow

  • Use workflow from 保持默认分支(通常为 master)
    Keep the default branch (usually master) in Use workflow from
  • 在 branch 输入框填写 PR 分支 fix/cromfs-getdents-bound
    Enter PR branch fix/cromfs-getdents-bound in the branch field
  • 设置需排除的文件/目录(目录请以"/"结尾)
    Set files/directories to exclude (directories should end with "/")
  1. 等待工作流完成 | Wait for the workflow to complete
    格式化后的代码将作为独立提交推送至你的分支。
    The formatting changes will be pushed to your branch as a separate commit.

完成后,提交将自动更新至 fix/cromfs-getdents-bound 分支,关联的 Pull Request 也会同步更新。
Once completed, commits will be pushed to the fix/cromfs-getdents-bound branch automatically, and the related Pull Request will be updated.

如有问题欢迎联系我们,再次感谢您的贡献!💐
If you have any questions, feel free to reach out. Thanks again for your contribution!

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

📌 Code Review Assignment

🏷️ Tag: components

Reviewers: @Maihuanyi

Changed Files (Click to expand)
  • components/dfs/dfs_v1/filesystems/cromfs/dfs_cromfs.c
  • components/dfs/dfs_v2/filesystems/cromfs/SConscript
  • components/dfs/dfs_v2/filesystems/cromfs/dfs_cromfs.c
  • components/dfs/dfs_v2/filesystems/cromfs/utest/Kconfig
  • components/dfs/dfs_v2/filesystems/cromfs/utest/SConscript
  • components/dfs/dfs_v2/filesystems/cromfs/utest/tc_cromfs_dirent.c
  • components/utilities/utest/Kconfig

📊 Current Review Status (Last Updated: 2026-10-21 14:10 CST)


📝 Review Instructions

  1. 维护者可以通过单击此处来刷新审查状态: 🔄 刷新状态
    Maintainers can refresh the review status by clicking here: 🔄 Refresh Status

  2. 确认审核通过后评论 LGTM/lgtm
    Comment LGTM/lgtm after confirming approval

  3. PR合并前需至少一位维护者确认
    PR must be confirmed by at least one maintainer before merging

ℹ️ 刷新CI状态操作需要具备仓库写入权限。
ℹ️ Refresh CI status operation requires repository Write permission.

Cover the check added by the previous commit without needing a block device or
zlib: cromfs can be mounted from a memory image through the data pointer of
dfs_mount(), which is how bsp/k230 and bsp/cvitek mount their root filesystem.
Listing a directory never decompresses file data, so the test stays free of the
optional zlib dependency.

The test builds a cromfs image in RAM (256 byte header plus one dirent) and
lists its root directory twice:

  1. Control entry, name_size = 2 -> getdents returns a positive byte count and
     d_namlen is 2, so valid entries are still accepted.
  2. name_size = 512, well above DIRENT_NAME_MAX -> getdents must return -EINVAL
     and must leave the caller's dirent untouched.

Verified on bsp/qemu-vexpress-a9 (RT_USING_DFS_V2 + RT_USING_DFS_CROMFS):

  without the fix : case 2 returns 260 and the 512 byte copy smashes the caller
                    stack, the test then dies with a prefetch abort
  with the fix    : case 1 returns 260 with d_namlen 2, case 2 returns -EINVAL,
                    no fault

components/dfs/dfs_v2/filesystems/cromfs/SConscript gains the usual recursion
into subdirectories so the new utest/ directory is picked up, and
components/utilities/utest/Kconfig sources its Kconfig.

Refs RT-Thread#11905
@Yunlk

Yunlk commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

回归测试补上了(commit 5d2ef22b),并且拿到了 master 与本 PR 的对照结果。

用例怎么做的

cromfs 支持通过 dfs_mount(..., "crom", ..., data) 的 data 指针直接读内存镜像(dfs_cromfs_mount 里走 cromfs_data_read_bytes),而列出目录不涉及解压,所以这个用例既不需要块设备、也不需要 zlib。

在 RAM 里构造 256 字节头 + 一个目录项:

  • control:name_size = 2,名字 "oo"
  • evil:name_size = 512,远大于 DIRENT_NAME_MAX(256)

分别挂到两个独立挂载点,dfs_file_open(O_DIRECTORY) 后调 dfs_file_getdents。

实验结果

master 1c801efa(无修复)
  control image  -> ret=260, d_namlen=2, name="oo"
  oversized name -> ret=260, d_namlen=0
  [E/utest] ASSERT ... msg: ((ret) not == (-22))
  prefetch abort:
  shutdown...
  (0) assertion failed at function:rt_hw_cpu_shutdown

本 PR(有修复)
  control image  -> ret=260, d_namlen=2, name="oo"
  oversized name -> ret=-22, d_namlen=0
  [  PASSED  ] [ result ] testcase (components.dfs.v2.cromfs.dirent_name)

两件事值得注意:

  1. 无修复时 getdents 返回 260(正常返回),512 字节被拷进 256 字节的 d_name,随后真的 prefetch abort 崩了——不是只有断言失败,是溢出确实打崩了调用方栈。这与 issue 里报告的现场一致。
  2. 有修复时返回 -EINVAL,调用方缓冲区没被写(d_namlen 保持 0),control 镜像仍然正常列出(260 / 2 / "oo"),说明校验没有误伤合法条目。

附带的构建改动

components/dfs/dfs_v2/filesystems/cromfs/SConscript 原本没有递归子目录,新增的 utest/ 不会被编译,所以补上了仓库里通用的递归写法;同时 components/utilities/utest/Kconfig 增加一行 rsource 让新 Kconfig 生效。

用例已过仓库 .clang-format 校验。这个用例不依赖硬件和 zlib,理论上可直接进 CI;是否要挂到 .github/utest/configs/ 下某一组,请维护者定。

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant