Repository navigation
[drivers][block] prevent arbitrary kernel write via BLK_PARTITION ioctl on RT-Smart - #11981
liangzhan5 wants to merge 3 commits into
Conversation
…tl on RT-Smart Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
|
👋 感谢您对 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-11-05 22:06 CST)
📝 Review Instructions
|
|
将lwp_self注释掉后在内核态测试(模拟用户态) 测试代码 修复前 修复后 |
BernardXiong
left a comment
There was a problem hiding this comment.
先看了一遍,方向没问题,PARTITION 这条确实堵上了,err = RT_EOK 那个补充也是对的。不过同类入口还有不少,安全修复还不完整,建议补齐再合。
必须处理
1. 同类的"用户指针直接写"还在(5 处)
用户态 ioctl 会把 arg 原样传到 rt_device_control()(syscall_device.c#L94-L97),除了已修的 PARTITION,下面这些仍然能直接写内核地址:
分区设备 blk_dev_control():
- GETGEOME blk_dev.c#L75-L90:直接写
geometry->bytes_per_sector / block_size / sector_count - SSIZEGET / ALL_BLK_SSIZEGET blk_dev.c#L120-L128 → blk_dfs.c#L259-L280,最后都是
rt_memcpy(args, ...)
整盘设备 blk_control():
- GETGEOME blk.c#L115-L125:
disk->ops->getgeome(disk, args),实现里就是rt_memcpy(geometry, ...)(比如 dev_block.c#L351-L359),等于把整个结构体写到用户给的指针 - SSIZEGET / ALL blk.c#L189-L197,同上
建议抽个 blk_copy_out(args, k, n),PARTITION / GETGEOME / SSIZEGET 都走它。本 PR 至少把两处 GETGEOME 一起修掉,SSIZEGET/ALL 可以另开 issue 跟踪,但最好也顺手。
2. 补 GETGEOME 的时候有个坑:内部调用点会误伤
blk_fops_open() 是在用户进程 open() 的上下文里,用内核指针调 GETGEOME / ALL_BLK_SSIZEGET 的(blk_dfs.c#L35-L37);device_get_blk_ssize() / device_get_all_blk_ssize() 内部也是用栈上的 &geometry 去调 GETGEOME(blk_dfs.c#L264、#L277)。
如果 GETGEOME 直接套 PR 现在这个 lwp_self() != RT_NULL 分支,这几处会被当成非法用户地址拒掉:blk_fops_open 拿不到 geometry,后面读写全错;SSIZEGET 里 geometry 变成未初始化,回给用户的是垃圾。
所以改 GETGEOME 之前,先把这三处内部调用改成直接调内核侧的取 geometry / 总大小的函数(放在 block 层里,按分区还是整盘分发),不要再经过会做用户拷贝的 control 路径。
另外 helper 本身的分流要写成:用户态 + 用户地址 → 拷贝;用户态 + 内核地址 → -EFAULT(这一步才是拦住攻击的关键,不能退化成 rt_memcpy);内核态 → 直接写。别只按 lwp_in_user_space() 判断地址范围,那样用户态传内核指针还是会漏。
3. SSIZEGET / ALL 没判空
ioctl(fd, RT_DEVICE_CTRL_BLK_SSIZEGET, NULL) 会一路走到 rt_memcpy(NULL, ...)(blk_dev.c#L120-L128 / blk.c#L189-L197),用户态就能把内核打挂,顺手加个判空吧。
4. clang-format 会挂
用仓库锁定的 clang-format 22.1.8 按 CI 的方式跑了一下(--style=file --lines=110:134),新代码 123~126 行 报 InsertBraces,if/else 要补花括号,不然 CI 会再追一个格式化提交。
小建议(不阻塞)
kpart这个临时变量可以省,直接lwp_put_to_user(args, &blk->partition, sizeof(blk->partition)),源本来就是内核内存,少一次拷贝。- 回拷的结构体里
lock是&disk->usr_lock(blk_partition.c#L78),属于内核指针。整结构体拷贝、保持 ABI 不变的前提下,把lock先清成 NULL 再拷,别把内核地址漏给用户。 - PR 描述有两处可以写准确点:
lwp_put_to_user()失败返回的是 0,不是 -EFAULT;另外它只做范围检查,跨页时理论上有部分写的可能(只会写进用户自己的地址空间,没有安全问题,但"不会执行任何写入"说得太绝对了)。 - 验证方式建议换一下:现在把
lwp_self()注释掉在内核态跑,实际只验证了"内核地址被_lwp_user_range_is_valid()拒掉"这条路径,成功路径(真实用户进程的 ioctl)没跑到。建议用真的 RT-Smart 用户进程跑一遍;顺带也能看到现在 GETGEOME 照样能写内核地址。 - 顺带发现一个不相关的既有问题:blk_dev.c#L92-L106 的 SYNC / ERASE / AUTOREFRESH 分支都没给
err赋值,永远返回-EINVAL,可以另开 issue。
结论:PARTITION 的修法本身没问题,建议把两条 GETGEOME(至少)和 SSIZEGET 的判空一起合进来。
|
…or dfs internal calls Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
改好了,用同样的测试方法通过了 我保留了kpart因为觉得lwp_put_to_user可能有竞态窗口,然后dfs.c里只改了open函数。 blk.c的blk_control也存在跟blk_dev.c同款的错误,所以我觉得可以用相同的方式把blk.c修了,尤其是同样地不再调用device_get_blk_ssize和device_get_all_blk_ssize(都不安全)。然后可以考虑把这两个函数从blk_dfs删了?似乎没人用它们了。 是否需要再提一个pr? 测试结果 |
Rbb666
left a comment
There was a problem hiding this comment.
最新修改引入了整盘设备打开路径的回归,需要先修正:
blk_dfs.c:37-42无条件to_blk(dev)后读取blk->disk,但rt_hw_blk_disk_register()也给整盘对象安装同一套device_set_blk_fops()。打开整盘节点时对象实际为rt_blk_disk,按rt_blk_device解释会取到错误指针并在disk->ops->getgeome()崩溃。请区分整盘与分区,提供明确的内核侧 geometry 查询接口供共同 fops 使用。- 分区的
ALL_BLK_SSIZEGET和file->vnode->size现在使用disk_geometry.sector_count,返回整盘容量。请按节点类型计算:整盘用磁盘扇区数,分区用blk->sector_count,保持计算宽度正确。 - 获取 geometry 失败时释放
data,但dev->user_data仍指向它。请在成功后再发布指针,或在失败路径清空。
此前指出的整盘 GETGEOME/SSIZEGET 用户指针路径也仍需处理。请使用真实 RT-Smart 用户进程,覆盖整盘、分区、合法缓冲区、非法指针及正常读写;仅注释掉 lwp_self() 的内核态测试无法覆盖这些路径。当前格式检查也未通过。
…isk devices Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
095e3d2 to
8a7213a
Compare
用与 复原了blk_dfs.c中open的逻辑,添加错误处理,把 删掉了 |
拉取/合并请求描述:(PR description)
[
为什么提交这份PR (why to submit this PR)
issue:#11952
RT_DEVICE_CTRL_BLK_PARTITION直接将用户态传入的裸指针作为 rt_memcpy 的目的地址,导致非特权进程可实现任意内核写。
rt_memcpy 执行完毕后未更新err,所以即使 ioctl 最终返回 -1,目标地址的内容已被更改。错误码与实际情况不符。
(RT_DEVICE_CTRL_BLK_GETGEOME等似乎也具有类似的问题)
你的解决方案是什么 (what is your solution)
在 RT_USING_LWP 条件下,通过 lwp_self() 判断当前是否为用户态上下文:
是用户态:先将分区数据拷贝到内核栈临时变量,再使用 lwp_put_to_user() 安全回拷至用户空间。若用户提供的指针非法,lwp_put_to_user 会返回 -EFAULT,且不会执行任何写入。
非用户态(内核内部调用):沿用原有的 rt_memcpy 路径,修正原代码中遗漏赋值 err = RT_EOK 的问题
请提供验证的bsp和config (provide the config and bsp)
/rt-thread/bsp/qemu-vexpress-a9
]
当前拉取/合并请求的状态 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