Repository navigation
[smart][lwp] validate IPC object ownership in syscalls - #12015
cms19859230182-lang wants to merge 1 commit into
Conversation
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
🏷️ Tag: components_lwpReviewers: @xu18838022837 Changed Files (Click to expand)
🏷️ Tag: kernelReviewers: @GorrayLi @ReviewSun @hamburger-os @lianux-mm @wdfk-prog @xu18838022837 Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-11-14 15:40 CST)
📝 Review Instructions
|
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
github-actions
Bot
added
RT-Smart
|
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
我按 handle 校验、引用计数、删除路径三部分过了一遍，整体方向是对的：把用户传入的裸指针收束为"必须在本 LWP 的对象 AVL 中、类型匹配、调用期间持有引用"，补上了 #11960 缺的那一层。delete 路径复用 lwp_user_object_delete()，AVL 查找只比较 key、不解引用，伪造指针进不到 rt_*_delete，这部分没有问题。以下两点建议在标记 ready 前确认。
1. timer 的 arg 仍然是裸用户指针，同类问题没有闭环
sys_rt_timer_control() 现在校验了 timer，但 arg 还是原样传给 rt_timer_control()。后者在 RT_TIMER_CTRL_GET_TIME 时执行 *(rt_tick_t *)arg = timer->init_tick，GET_STATE 也会写回，SET_TIME 则读取 *(rt_tick_t *)arg。用户可以把 arg 指向任意内核地址，效果和 #11960 里的原语同级。
既然 PR 声称覆盖了 timer 家族，建议一并收掉：按 cmd 用 lwp_get_from_user() / lwp_put_to_user() 拷贝参数，或者先做 cmd 白名单，只放行不需要指针的命令。
2. delete 与阻塞等待的语义变化、以及引用可能收不回来
引入 in-flight 引用后，sem_delete / mutex_delete 不再调用 rt_*_delete()，也就不会再唤醒挂在该对象上的等待者。PR 描述里已经写明了这个 trade-off，但对 RT_WAITING_FOREVER 的等待者来说，结果是永久挂起，而以前会以 -RT_ERROR 返回。
更需要留意的是异常路径：线程阻塞在 rt_sem_take_interruptible() 时被 sys_thread_delete() 强杀，或进程退出时线程直接消失，syscall 都回不到 lwp_user_object_put()，这份引用就永远收不回来，内核对象随之泄漏。建议确认这种情况下有兜底（例如 delete 时把对象标记为 dying 并唤醒等待者，或在 defunct 线程清理时归还引用）；如果确认这是可接受的约束，至少写进注释里。
小建议（不阻塞）
lwp_user_object_get()里的found可以省掉，命中时直接return object，否则返回RT_NULL，读起来更直接。- 12 个 syscall 的 get/call/put 模板重复度偏高。C 里显式展开没问题，如果想去重，可以抽一个很小的宏；不建议为此引入更重的封装。
Overall the approach is sound: a user handle must now resolve to an object in the calling LWP's AVL, match the expected class, and stay referenced for the duration of the syscall. That closes the gap in #11960. Delete paths are fine as-is, since lwp_user_object_delete() only compares AVL keys and a forged pointer never reaches rt_*_delete().
1. sys_rt_timer_control() still passes arg through unchecked. rt_timer_control() writes *(rt_tick_t *)arg for GET_TIME / GET_STATE and reads it for SET_TIME, so a user can point arg at a kernel address and obtain the same arbitrary read/write primitive. Since the PR claims to cover the timer family, please either copy arg with lwp_get_from_user() / lwp_put_to_user() per command, or restrict the accepted commands to a pointer-free whitelist.
2. Delete vs. blocked waiters changed, and the reference can leak. With the in-flight reference, sem_delete / mutex_delete no longer reach rt_*_delete(), so waiters are no longer resumed (the description already notes this). For RT_WAITING_FOREVER waiters that means a permanent hang instead of -RT_ERROR. More importantly: if a thread is force-deleted through sys_thread_delete() while blocked in rt_sem_take_interruptible(), or the process exits, the syscall never returns and lwp_user_object_put() never runs — the in-flight reference, and with it the kernel object, leaks. Please confirm there is a fallback (e.g. mark the object dying on delete and wake waiters, or drop the reference from defunct-thread cleanup).
Minor, non-blocking: found in lwp_user_object_get() can be dropped; and the 12 get/call/put blocks could optionally be folded into a small helper macro.
这两个问题是我设置draft的原因，我晚上回寝室发现指针有bug，所以就设置了 然后第二个问题我确实没有看出来，感谢指正🙏🙏🙏 |
1c5a9c0 to
4c3811d
Compare
|
@BernardXiong 两点已改，仍是一笔提交（
|
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
复核 4c3811d9，之前提的两点都已落实，可以合并。
已确认的修复
- timer
arg已收口：sys_rt_timer_control()不再把用户指针交给rt_timer_control()。SET_TIME经lwp_get_from_user()拷入，GET_TIME/GET_REMAIN_TIME/GET_STATE/GET_FUNC/GET_PARM成功后lwp_put_to_user()拷回，SET_FUNC与未知 cmd 直接-EINVAL，SET_PARM只传值不解引用；__exit统一lwp_user_object_put()。原先"用户可让内核读写任意地址"的原语已关闭，-EFAULT路径也处理了。 - delete 唤醒：
_object_wakeup_waiters()只在 AVL 命中后调用，伪造句柄仍只作为 key 比较、不会解引用；按类型 resumesuspend_thread（mb/mq 含suspend_sender_thread）。顺序是先唤醒、再_object_node_delete()减引用，最后一个引用释放时才_object_destroy()，因此不会出现"唤醒后对象立刻被销毁"的 UAF，等待者返回后put()也是安全的。
非阻塞观察
RT_TIMER_CTRL_GET_FUNC仍会把timer->timeout_func的内核地址拷回用户态（src/timer.c:728），等于保留了一个信息泄露面，而用户态对它有合法用途的可能性很低。建议与SET_FUNC一样返回-EINVAL；GET_PARM返回的是 parameter 值，保持现状即可。_object_wakeup_waiters()与并发rt_sem_take()之间还有窄窗口：某线程已经lwp_user_object_get()拿到引用、但尚未挂入suspend_thread时，删除路径的唤醒会漏掉它，之后它会永久阻塞并占住引用。相比原实现（在已释放对象上挂起）是改善，但建议把这条与强杀泄漏一起写进lwp_user_object_delete()的注释，明确剩余约束。
Re-checked 4c3811d9: both requested changes are in place, no blockers.
1. timer arg is now marshalled. sys_rt_timer_control() copies SET_TIME in via lwp_get_from_user(), copies GET_* results back via lwp_put_to_user(), rejects SET_FUNC and unknown commands with -EINVAL, and passes SET_PARM by value. The arbitrary kernel read/write primitive is closed, and -EFAULT is handled. The reference is released on every exit path.
2. Waiters are resumed on delete. _object_wakeup_waiters() is only reached after the AVL lookup succeeds, so a forged handle is still never dereferenced. It resumes suspend_thread (plus suspend_sender_thread for mb/mq) per type. Waking happens before _object_node_delete() drops the node's reference, and destruction is deferred to the last reference, so there is no use-after-wake UAF.
Non-blocking: RT_TIMER_CTRL_GET_FUNC still copies a kernel text address (timer->timeout_func) back to userspace; consider rejecting it like SET_FUNC. Also, a concurrent rt_sem_take() that has already taken its reference but not yet suspended can slip past the wakeup and block forever — better than the old behaviour, but worth documenting next to the accepted force-kill leak.
4c3811d to
c1228dd
Compare
|
@BernardXiong 已按非阻塞建议更新至
请按新 tip 复看这两项非阻塞点。 |
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
复核 c1228dd1：相对已批准的 4c3811d9，增量只是让 GET_FUNC 与 SET_FUNC 一样返回 -EINVAL，并在 _object_wakeup_waiters() 旁补充强杀/进程退出引用残留的说明，符合上一轮建议。handle 归属/类型/引用校验、timer arg 封送、删除唤醒逻辑静态检查未发现阻塞问题。
非阻塞：RT_TIMER_CTRL_SET_TIME 把用户 tick 直接交给 rt_timer_control()，内核里有 RT_ASSERT((*(rt_tick_t *)arg) < RT_TICK_MAX / 2)；开启断言时用户传超大值会触发内核 assert。建议 syscall 侧先做同样的范围检查并返回 -EINVAL。
rt-thread/components/lwp/syscall/syscall_rt_ipc.c
Line 1372 in c1228dd
CI：29 个失败全部停在 Install Tools / Install ToolChains / Install toolchain 等工具链下载步骤（网络故障），不是代码或测试失败；当前 head 的 CI 实际未跑起来，建议 rerun 后再合并。
Rbb666
left a comment
Rbb666
left a comment
There was a problem hiding this comment.
静态审查发现两处可触发的安全问题，以及删除和线程退出路径上的三处回归。建议合并前处理，具体场景见行内评论。
| break; | ||
| case RT_TIMER_CTRL_SET_PARM: | ||
| /* arg is the parameter value itself, it is not dereferenced */ | ||
| karg = arg; |
There was a problem hiding this comment.
[P1] 校验定时器回调使用的信号量
SET_PARM 接受任意指针，sys_rt_timer_create() 也直接保存 data。到期后，timer_timeout_callback() 将它当作 rt_sem_t 传给 rt_sem_release()。用户可以用自己的定时器操作其他 LWP 的信号量，或传入无效指针触发内核故障。请在创建和更新参数时都校验归属和类型，并确保回调结束前信号量仍有有效引用。
| rt_mq_t mq = (rt_mq_t)object; | ||
|
|
||
| level = rt_spin_lock_irqsave(&mq->spinlock); | ||
| rt_susp_list_resume_all(&mq->parent.suspend_thread, RT_ERROR); |
There was a problem hiding this comment.
[P1] 避免把删除错误当作已接收的消息长度
此处写入的正值 RT_ERROR 经 _rt_mq_recv() 原样返回，sys_mq_recv() 则把正返回值用作拷贝长度。启用 libc 时 RT_ERROR 为 255，等待者传 size=1 且提供 255 字节可写的用户缓冲区，就可能从未填充的内核缓冲区拷出 255 字节。原有最终删除路径也有这个问题，新增唤醒还会在 fork() 后仅关闭一个共享句柄时触发。请在接收路径把唤醒错误转换成负返回值。rt_susp_list_resume_all() 会忽略负错误参数。
| * in-flight reference. | ||
| * 2) Concurrent window: a caller that has called | ||
| * lwp_user_object_get() but has not yet queued itself on | ||
| * suspend_thread can miss this wakeup, then block forever and |
There was a problem hiding this comment.
[P2] 防止删除与入队之间丢失唤醒
线程 A 在空信号量上调用 sem_take(RT_WAITING_FOREVER)，取得引用后被抢占。线程 B 删除最后一个句柄，此时队列为空。A 恢复后才入队，此后可永久阻塞，且自身引用会阻止对象回收。请让入队检查和关闭状态转换在同一把 IPC 锁下同步。
| if (lwp_avl_find((avl_key_t)object, lwp->object_root) && | ||
| (rt_object_get_type(object) == type)) | ||
| { | ||
| rt_atomic_add(&object->lwp_ref_count, 1); |
There was a problem hiding this comment.
[P2] 在线程被强制删除时释放 syscall 引用
sys_thread_delete() 可以直接销毁阻塞在 sem_take() 的线程，syscall 尾部的 lwp_user_object_put() 不会执行。删除最后一个句柄后对象仍然无法回收，重复创建对象、阻塞线程再删除的过程会持续消耗内核堆。请让线程清理路径归还仍持有的 syscall 引用。
| * suspend_thread can miss this wakeup, then block forever and | ||
| * pin that reference. | ||
| */ | ||
| _object_wakeup_waiters(object); |
There was a problem hiding this comment.
[P2] 保留其他 LWP 对共享 IPC 的等待
fork() 会为子进程注册同一个 IPC 对象。子进程在继承的空信号量上等待时，父进程删除自己的句柄会在这里唤醒整个等待队列，导致子进程收到错误，尽管它的句柄仍然有效。请在最后一个所有者释放时才执行全队列唤醒。
c1228dd to
f44ffb3
Compare
|
@Rbb666 @BernardXiong 已按本轮意见更新，tip：
另已恢复 |
行，让我再看看 |
cpp check可以先不管 |
| ret = rt_timer_delete((rt_timer_t)object); | ||
| if (ret == RT_EOK) | ||
| { | ||
| lwp_timer_callback_data_free(callback_data); |
There was a problem hiding this comment.
[P1] 释放回调上下文前需要同步已派发的回调
rt_timer_delete() 只在 timer-list 锁下移除定时器，没有等待已派发回调结束。_timer_check() 在 src/timer.c:534 释放该锁后才调用回调。SMP 下，回调可以已经拿到 parameter，但尚未执行 lwp_timer_callback() 中的 data->lock，另一 CPU 就在这里释放了上下文和其信号量引用。回调继续后会对已释放的 data->lock 加锁，并读取 data->sem。可被抢占的软定时器回调也有这个窗口。请让派发路径持有有效生命周期引用，并在回调退出后再释放上下文。
|
|
||
| level = rt_spin_lock_irqsave(&(mq->spinlock)); | ||
|
|
||
| if (_ipc_object_is_closing(&mq->parent.parent)) |
There was a problem hiding this comment.
[P2] 每次准备入队前都需要检查 closing
这里仅在首次加锁时检查关闭状态。接收线程 A 被发送操作以 RT_EOK 唤醒后，线程 B 可以先取走消息，再关闭最后一个句柄。关闭操作看到 A 已离开等待队列，随后 A 才在第 3983 行重新加锁。队列此时为空，A 会直接进入下一轮并以 RT_WAITING_FOREVER 再次入队，永久阻塞并持有对象引用。Mailbox 收发和 MQ 等待发送也有相同结构。请在每次入队前、同一 IPC 锁下检查 closing，覆盖成功唤醒后的重试。
|
|
||
| #define GET_MESSAGEBYTE_ADDR(msg) ((struct rt_mq_message *)msg + 1) | ||
|
|
||
| static rt_bool_t _ipc_object_is_closing(rt_object_t object) |
There was a problem hiding this comment.
[P2] 在预处理阶段移除非 Smart 构建的 closing 检查
关闭 RT_USING_SMART 时，此函数及新增调用仍会参与编译。hk32/hk32f030c8-mini 使用 -O0，恒假的函数调用和分支仍有代码开销。准确基线 611d7c07 的 devices.soft_i2c 配置使用 65,472 B ROM，本 PR 的相同配置使用 65,552 B，导致 64 KB ROM 超限 16 B。请将 Smart 专用检查在非 Smart 构建中编译掉，并确认该 BSP 构建恢复。全局 MQ 错误符号转换也有代码增量，80 B 总增量需要通过构建继续确认。
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
CI 结果（head f44ffb36）：两项代码侧失败，本轮不做完整 review。
- 代码格式：
Scan code format and license的Check clang-format失败，报错集中在 include/rtdef.h:307-316，即本 PR 新增的lwp_ref_count/lwp_owner_count/lwp_flags字段以及#ifdef RT_USING_SMART边界。请用仓库固定的 clang-format 版本重新格式化。 - BSP 链接失败：
others_ft32_mm32_acm32中bsp/hk32/hk32f030c8-mini链接报.text will not fit in region ROM，ROM overflowed by 16 bytes（65552 B / 64 KB，100.02%）。这是该 BSP 已接近 ROM 上限，本 PR 在通用 kernel 路径增加的代码把它推过了边界；请确认能否精简，或由 BSP 维护者调整配置。
另外已确认上一轮的非阻塞建议已落实：sys_rt_timer_control() 在 SET_TIME 前增加了 tick >= RT_TICK_MAX / 2 的 -EINVAL 检查。CI 通过后再做完整复核。
Summary (EN): Two code-side CI failures: clang-format violations in the new include/rtdef.h fields, and a 16-byte ROM overflow when linking hk32/hk32f030c8-mini. The previous non-blocking SET_TIME range-check suggestion is now implemented. Full re-review will follow a green CI.
Rbb666
left a comment
Rbb666
left a comment
There was a problem hiding this comment.
复核当前提交 f44ffb3，上一轮行内指出的阻塞问题仍存在，需要修改后再合并：
- [P1] 定时器销毁未同步已派发回调，释放 callback_data 后回调仍可能访问 data->lock/data->sem。请建立派发到回调退出期间的有效生命周期保护，见原行内意见。
- [P2] Mailbox/MQ 成功唤醒后的重试路径会绕过 closing 检查，可能在最后所有者关闭后再次永久入队。请在每次准备入队时持同一 IPC 锁检查，见原行内意见。
- [P2] 强制删除阻塞线程仍不会归还在途对象引用，重复执行会持续泄漏内核堆；注释不能替代回收路径，见原行内意见。
此外，当前 CI 的格式检查和 hk32f030c8-mini 链接仍失败（ROM 超出 16 字节）；请处理非 Smart 构建增量，再补充删除与重试、回调与销毁并发的回归验证。为避免重复讨论，本轮沿用已有行内位置。
f44ffb3 to
aa236a6
Compare
|
@Rbb666 @BernardXiong 已按意见改完，tip：。
|
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
结论：格式 CI 在当前 head 仍失败。 / Verdict: format CI still fails at this head.
Scan code format and license 在 aa236a60b3 失败，日志报告 include/rtdef.h 的枚举与结构体区域 clang-format 违规；diff 里也能看到 struct rt_object 被整段改成 8 空格缩进（rtdef.h:293-318），与仓库格式不一致。上一条评论说“rtdef.h 格式 CI 已过”，但该 head 的 CI 结果并非如此。
失败 job：https://github.com/RT-Thread/rt-thread/actions/runs/43417230407/job/131858086043
建议用仓库固定的 clang-format 22.1.8（tools/ci/requirements-format.txt）对本次改动的 6 个文件重跑，只提交格式差异，避免夹带整段缩进变化。
另外，Rbb666 在 f44ffb36 上提出的两条行内意见（timer 回调生命周期保护、成功唤醒后重试绕过 closing 检查）本轮没有做完整复核；格式 CI 修好后需要一起确认。
Format CI still fails on include/rtdef.h at this head, including an unintended whole-struct reindent. Please run the pinned clang-format 22.1.8 and re-submit only the real formatting delta.
Fix RT-Thread#11960 Resolve user-supplied IPC handles through the calling LWP's object AVL, require the expected object class, and hold an in-flight reference until the syscall returns. Marshal timer control arguments between user and kernel memory, and reject callback-function commands and unknown commands. Validate the semaphore used by timer creation and SET_PARM. Keep one semaphore reference in the timer callback data until parameter replacement or timer destruction, and serialize callback release with those paths. Reject SET_TIME values greater than or equal to RT_TICK_MAX / 2 before the kernel timer assertion can be reached. Return negative message-queue receive errors after deletion wakeups so a positive RT_ERROR is never treated as a received message length. Track LWP owners separately from in-flight references, and close and wake waiters only when the final owner releases an object. Set the closing flag under the object's IPC lock and check it under the same lock before blocking, closing the delete-versus-suspend window. Initialize Smart object bookkeeping in both static and dynamic object creation paths. The remaining force-kill limitation is documented at lwp_user_object_get(): without per-thread syscall reference bookkeeping, a thread destroyed before returning from its syscall cannot release that reference. No new tracking framework is introduced for that case.
aa236a6 to
8fc1380
Compare
|
@BernardXiong @Rbb666 rtdef.h 已按仓库 clang-format 重整；timer 生命周期和 closing 重试、强删归还也补上了。 |
中文
问题
Smart LWP 的 IPC syscall 曾直接把用户句柄当内核对象指针使用；timer 参数封送、回调信号量生命周期、delete 唤醒和共享句柄关闭也存在后续审稿指出的边界问题。
改动
SET_PARM只接受本 LWP 的 semaphore，回调数据持有引用并与参数替换、销毁串行化；SET_TIME拒绝tick >= RT_TICK_MAX / 2。验证
静态逐项核对本轮 P1、P2 与
SET_TIME意见；clang-format 22.1.8检查改动区域，git diff --check通过。未跑板。剩余约束
现有强杀和 defunct 清理没有逐 syscall 引用归还点，也没有逐线程引用表，因此线程若在 syscall 返回前被销毁，其在途引用仍可能残留。该约束已写入
lwp_user_object_get()注释，本次未引入新的跟踪框架；正常 delete 唤醒路径会返回并释放引用。English
Problem
Smart LWP IPC syscalls once treated user handles as raw kernel pointers. Follow-up review also found timer argument marshalling, callback semaphore lifetime, delete wakeup, and shared-handle close edge cases.
Changes
SET_PARM; hold its reference in serialized callback data until replacement or destruction. RejectSET_TIMEwhentick >= RT_TICK_MAX / 2.Test plan
Statically checked each P1, P2, and
SET_TIMEreview item; checked changed regions withclang-format 22.1.8and rangit diff --check. No board run.Remaining constraint
The current force-kill and defunct cleanup paths have no per-syscall release hook or per-thread reference table. A thread destroyed before returning from a syscall can therefore still leave its in-flight reference behind. This is documented at
lwp_user_object_get(); no new tracking framework is introduced. The normal delete-wakeup path returns and releases its reference.Fixes #11960