Repository navigation
[dfs][nfs] bound READ copy length to request and data_len - #12012
Conversation
|
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
结论 / Verdict: 修改方向正确，未发现新的阻塞问题。 / The fix is sound; no new blocking issue found.
已验证 / Verified
nfs_read()将拷贝长度限制为min(server count, args.count, data_len)，memcpy不会越过调用者缓冲区或实际解码的数据；buf/count/ offset 按实际拷贝字节推进，短读不会再跳数据。 / Copy length is bounded by both the request and the decoded opaque length; pointers advance by actual bytes.xdr_READ3resok()把 READ opaque 上限收紧为 1024，超大 READ 回复会解码失败。 / XDR decode rejects READ payloads larger than 1024.WRITE3resok.count被 clamp 到提交长度，服务端无法把写偏移推进到未发送的位置。 / Write offset cannot advance past submitted length.- CI：104 success / 2 skipped。
建议（非阻塞） / Non-blocking suggestions
- nfs_xdr.c:21 的
NFS_READ_MAX_DATA与 dfs_nfs.c:22 的DFS_NFS_MAX_MTU重复。建议抽到共享头文件复用，避免以后只改一处导致合法 READ 被解码拒绝。 / Duplicated 1024 limit; share one definition to avoid future divergence. - dfs_nfs.c:677-678 在发送前按
args.count推进buf/count，而 NFS 允许WRITE3resok.count < args.count。发生合法的短写时，未写出的尾部会被跳过，下一轮会把后续数据写到更小的 offset，造成数据错位。这是既有逻辑，但既然本 PR 已处理WRITE3resok.count，建议一并按实际bytes推进并在非完整写入时重试剩余数据。 / On a legal short write, the unwritten tail is skipped and later data lands at the wrong offset. Pre-existing, but worth fixing together with the new clamp.
Address BernardXiong's non-blocking review comments on RT-Thread#12012: - Move DFS_NFS_MAX_MTU into dfs_nfs.h and use it both to cap WRITE/READ request sizes and as the opaque length limit in xdr_READ3resok(), replacing the duplicated NFS_READ_MAX_DATA definition. The request cap and reply decoder can no longer drift apart. - In nfs_write(), stop advancing buf/count by args.count before the RPC. WRITE3resok.count may legitimately be smaller than the submitted count; now buf/count/offset/total advance by the clamped accepted byte count, so the unacknowledged tail is retried at the correct offset instead of being skipped. A zero-progress reply breaks the loop instead of spinning forever and returns the bytes accepted so far.
📌 Code Review Assignment🏷️ Tag: componentsReviewers: @Maihuanyi Changed Files (Click to expand)
📊 Current Review Status (Last Updated: 2026-11-12 00:39 CST)
📝 Review Instructions
|
BernardXiong
left a comment
BernardXiong
left a comment
There was a problem hiding this comment.
Conclusion: sound; no blocking issue found.
Verified on 30968ae8:
nfs_write()clampsWRITE3resok.countto the submitted length and advancesbuf/countonly by the accepted bytes, so a short write retries the tail at the correct offset and a zero-count reply cannot spin.- READ and WRITE now share
DFS_NFS_MAX_MTUfromdfs_nfs.h; the include is valid, andxdr_READ3resok()caps the decoded opaque at 1024. - The new
bytes == 0break still reaches the single outerxdr_free()(the bundledxdr_bytes()clears the pointer inXDR_FREEmode), so there is no double free on this path. - CI: 107 pass / 3 skipping.
|
看了一遍实现，核心修法是对的。下面是我逐条核过的结论，加两个观察（都不是本次改动引入的，但正好在这个函数里）。 核对通过的部分1. 拷贝长度收敛正确 bytes = res.READ3res_u.resok.count;
if (bytes > args.count) bytes = args.count; /* 不超请求 */
if (bytes > res...data.data_len) bytes = data.data_len; /* 不超已解码 */
2. xdr_bytes(..., DFS_NFS_MAX_MTU) /* 原来是 ~0 */
3. 零进度保护
4. 原来 两个观察（
|
Fix RT-Thread#11917 nfs_read() used the server-controlled READ3resok.count as the memcpy length without checking it against the request or the decoded data. Clamp each copy to min(server count, args.count, data.data_len), advance buf and the remaining count by the bytes actually copied, and break out when a reply makes no progress so the loop cannot spin. Share DFS_NFS_MAX_MTU (1024) in dfs_nfs.h for both READ/WRITE request caps and the xdr_READ3resok() opaque decode limit, replacing the separate NFS_READ_MAX_DATA definition. In nfs_write(), clamp WRITE3resok.count to the submitted length and advance buf/count/offset only by the accepted bytes so short writes retry the tail correctly and zero-count replies stop the loop. Follow-up for Yunlk's review observations on nfs_read(): - When an RPC or NFS status failure occurs after earlier iterations already copied bytes and advanced fd->offset, keep total and return the bytes copied so far instead of resetting total to 0; only set total = -1 when total == 0. - Remove the stale placeholder comment inside the eof handling branch where fd->eof = TRUE and break are already performed.
30968ae to
e7375ad
Compare
|
@Yunlk 已按两条观察压入单提交更新（
恶意 NFS 服务端 PoC 本轮不做，上述长度边界静态结论仍然成立。 |
|
👋 感谢您对 RT-Thread 的贡献！Thank you for your contribution to RT-Thread! 为确保代码符合 RT-Thread 的编码规范，请在你的仓库中执行以下步骤运行代码格式化工作流（如果格式化CI运行失败）。 🛠 操作步骤 | Steps
完成后，提交将自动更新至 如有问题欢迎联系我们，再次感谢您的贡献！💐 |
中文
问题
NFS READ 曾信任服务器
count做memcpy，可越过调用方缓冲与已解码数据（#11917）。同函数里，部分读成功后再 RPC 失败会把total清零，调用方看到 0 字节却丢了 offset；另有过期 eof 占位注释。改动
min(请求, 已解码 data_len, 服务器 count)，xdr READ opaque 上限与请求侧共用DFS_NFS_MAX_MTUbuf/count/ offset；WRITE 短写同样按实际bytes推进验证
静态核对长度收敛与失败路径返回值；未跑板、未做恶意 NFS PoC。
English
Problem
NFS READ trusted the server
countformemcpy, which could overrun the caller buffer and decoded opaque (#11917). In the same path, a later RPC failure zeroedtotalafter a partial success, and a stale eof placeholder comment remained.Changes
min(request, decoded data_len, server count); shareDFS_NFS_MAX_MTUwith the xdr READ opaque capbuf/count/ offset by actual bytes; same for short WRITETest plan
Static check of the clamp and failure return path; no board run and no malicious NFS PoC.
Fixes #11917