feat(vfs): 添加路径遍历和目录切换的权限检查 #1348
Conversation
- 新增permission模块处理inode权限检查 - 在路径遍历和chdir/fchdir系统调用中添加权限验证
There was a problem hiding this comment.
Pull Request Overview
This PR implements VFS permission checking infrastructure for DragonOS, adding UNIX DAC (Discretionary Access Control) permission checking capabilities. The changes enable proper permission validation for directory access operations and file mode changes.
- Introduces a new
permission.rsmodule with core permission checking functions (inode_permission,check_chdir_permission) - Integrates permission checks into
chdirandfchdirsyscalls to validate directory access rights - Implements the
fchmodatsyscall functionality to modify file permissions while preserving file type bits
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| kernel/src/filesystem/vfs/permission.rs | New module providing UNIX DAC permission checking with capability override support |
| kernel/src/filesystem/vfs/syscall/sys_chdir.rs | Adds permission validation using check_chdir_permission and fixes typo in error code documentation |
| kernel/src/filesystem/vfs/syscall/sys_fchdir.rs | Adds permission validation for fchdir syscall |
| kernel/src/filesystem/vfs/open.rs | Implements fchmodat functionality to modify file permissions |
| kernel/src/filesystem/vfs/mod.rs | Integrates permission checks during path traversal and reorganizes imports |
Comments suppressed due to low confidence (1)
kernel/src/filesystem/vfs/syscall/sys_chdir.rs:106
- The directory type check is redundant. The
check_chdir_permissionfunction on line 98 already performs this validation on line 176 of permission.rs, returningENOTDIRif the file type is not a directory. This duplicate check should be removed to avoid redundant validation and improve code maintainability.
if metadata.file_type == FileType::Dir {
proc.basic_mut().set_cwd(new_path);
proc.fs_struct_mut().set_pwd(inode);
return Ok(0);
} else {
return Err(SystemError::ENOTDIR);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if metadata.file_type != FileType::Dir { | ||
| return Err(SystemError::ENOTDIR); | ||
| } |
There was a problem hiding this comment.
The directory type check is redundant. The check_chdir_permission function on line 47 already performs this validation on line 176 of permission.rs, returning ENOTDIR if the file type is not a directory. This duplicate check should be removed to avoid redundant validation and improve code maintainability.
| if metadata.file_type != FileType::Dir { | |
| return Err(SystemError::ENOTDIR); | |
| } |
|
这里需要fmt一下 |
| pub fn inode_permission( | ||
| metadata: &Metadata, | ||
| cred: &Arc<Cred>, | ||
| mask: u32, | ||
| ) -> Result<(), SystemError> { | ||
| // 从 mode 中提取权限位 | ||
| let mode_bits = metadata.mode.bits(); | ||
| let file_mode = mode_bits & 0o777; // 只保留 rwxrwxrwx | ||
|
|
||
| // 确定要检查哪组权限位 | ||
| let perm = if is_owner(metadata, cred) { | ||
| // 所有者权限（第 6-8 位） | ||
| (file_mode >> 6) & 7 | ||
| } else if in_group(metadata, cred) { | ||
| // 组权限（第 3-5 位） | ||
| (file_mode >> 3) & 7 | ||
| } else { | ||
| // 其他用户权限（第 0-2 位） | ||
| file_mode & 7 | ||
| }; | ||
|
|
||
| // PermissionMask 的低 3 位已经是 Unix 权限位格式 (rwx) | ||
| let need = mask & 0b111; | ||
|
|
||
| // 检查权限位是否满足请求 | ||
| if (need & !perm) == 0 { | ||
| return Ok(()); // 通过普通检查，权限允许 | ||
| } | ||
|
|
||
| // 尝试 capability 覆盖（类似 Linux 的 capable_wrt_inode_uidgid） | ||
| if try_capability_override(metadata, cred, mask) { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // 所有检查都失败 | ||
| Err(SystemError::EACCES) |
There was a problem hiding this comment.
权限相关的bit，本身就定义了mask，我觉得这个在这里写这么多位运算不是很好。and..我印象中好像内核有地方也写了类似的逻辑？可以再检查一下，完善一下这里。
There was a problem hiding this comment.
类似的逻辑指的是？是在内核中有相同作用的模块了吗？还是说这些位运算的逻辑？
There was a problem hiding this comment.
类似的逻辑指的是？是在内核中有相同作用的模块了吗？还是说这些位运算的逻辑？
指的是位运算。这些mask之类的，
这个结构体里面就有
- 重构权限位计算逻辑使用ModeType常量 - 更新gvisor测试白名单添加chdir相关测试
|
失败的测试用例: [1970-01-01T00:02:34Z INFO runner::lib_sync] |
- 将inode_permission及相关辅助函数从独立函数重构为Cred结构体方法 - 更新sys_chdir和sys_fchdir调用权限检查的新接口 - 优化代码组织结构，减少模块间依赖 Signed-off-by: longjin <longjin@DragonOS.org>
2 participants
Note
Add UNIX-style permission checks to path traversal and chdir/fchdir, implement chmod via fchmodat, and only fallback
arch_prctlto common handler on EINVAL; update tests.vfs/permission.rswithPermissionMaskandCred::{inode_permission,check_chdir_permission}including capability overrides.IndexNode::do_lookup_follow_symlink.chdir/fchdir: validate directory and permissions viacheck_chdir_permissionbefore switching CWD.do_fchmodat: update only permission bits, preserve file type, and apply viaset_metadata.arch_prctl: call x86_64 handler first; fallback to common handler only onEINVAL.chdir_test,fchdir_test; blocklistFchdirTest.PermissionDenied.Written by Cursor Bugbot for commit 8faea7f. This will update automatically on new commits. Configure here.