Skip to content

Conversation

@kaleidoscope416
Copy link
Contributor

Implementation Details
· 增加sys_chroot系统调用,将pcb中root改为指定路径

Follow-ups

· 校验/调整 CWD 以确保位于新根内(可选自动 chdir("/") 策略)
· 限制已打开 fd 访问旧根(fd 语义隔离)
· 补充文档与测试(含符号链接循环等边界)
· 与 mount/user namespace 协同的后续支持

Copilot AI review requested due to automatic review settings November 15, 2025 13:21
@github-actions github-actions bot added the enhancement New feature or request label Nov 15, 2025
Copilot finished reviewing on behalf of kaleidoscope416 November 15, 2025 13:24
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements the sys_chroot system call, which allows changing the root directory of the calling process. The implementation adds capability-based permission checking (CAP_SYS_CHROOT) and integrates with the existing VFS infrastructure.

Key changes:

  • New sys_chroot.rs handler with capability validation, path resolution, and root directory switching
  • Module registration in mod.rs to expose the new syscall handler
  • Test configuration updates enabling chroot_test with blocked edge cases

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
kernel/src/filesystem/vfs/syscall/sys_chroot.rs Implements the chroot syscall handler with capability checks, path validation, symlink following, and root directory update
kernel/src/filesystem/vfs/syscall/mod.rs Registers the new sys_chroot module
user/apps/tests/syscall/gvisor/whitelist.txt Enables chroot_test in the test suite
user/apps/tests/syscall/gvisor/blocklists/chroot_test Blocks 4 advanced chroot test cases (permission checks and /proc escape prevention)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


// 更新进程的 fs 视图:设置新的 root。
// 同时将工作目录移动到新根,保证位于 chroot 内部。
pcb.fs_struct_mut().set_root(inode);
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment states "同时将工作目录移动到新根,保证位于 chroot 内部" (simultaneously move the working directory to the new root to ensure it's inside the chroot), but the code only calls set_root(inode) without updating the working directory. This means the current working directory (CWD) could remain pointing to a path outside the new root, creating a potential security issue.

To fix this, you should also update the working directory. For example:

// 更新进程的 fs 视图:设置新的 root。
// 同时将工作目录移动到新根,保证位于 chroot 内部。
let fs_struct = pcb.fs_struct_mut();
fs_struct.set_root(inode.clone());
fs_struct.set_pwd(inode);
pcb.basic_mut().set_cwd(String::from("/"));
Suggested change
pcb.fs_struct_mut().set_root(inode);
let fs_struct = pcb.fs_struct_mut();
fs_struct.set_root(inode.clone());
fs_struct.set_pwd(inode);
pcb.basic_mut().set_cwd(String::from("/"));

Copilot uses AI. Check for mistakes.
// 查找并跟随符号链接
let root_inode = pcb.fs_struct().root();
let inode = match root_inode.lookup_follow_symlink(&path, VFS_MAX_FOLLOW_SYMLINK_TIMES)
{
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error from lookup_follow_symlink is being discarded and always converted to ENOENT. This could hide other important errors such as EACCES (permission denied), ELOOP (too many symbolic links), or other filesystem errors. While this pattern is consistent with other syscalls in the codebase (like sys_chdir), it makes debugging more difficult and could confuse users who encounter permission issues but receive "file not found" errors instead.

Consider propagating the actual error or at least handling common cases explicitly:

let inode = root_inode
    .lookup_follow_symlink(&path, VFS_MAX_FOLLOW_SYMLINK_TIMES)
    .map_err(|e| match e {
        SystemError::EACCES => SystemError::EACCES,
        SystemError::ELOOP => SystemError::ELOOP,
        _ => SystemError::ENOENT,
    })?;
Suggested change
{
{
Err(SystemError::EACCES) => return Err(SystemError::EACCES),
Err(SystemError::ELOOP) => return Err(SystemError::ELOOP),

Copilot uses AI. Check for mistakes.
@kaleidoscope416 kaleidoscope416 marked this pull request as draft November 18, 2025 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant