Refactor kernel CPU and clock frequency management#257
Conversation
- Replace static mutable NUM_CPU and CLOCK_FREQ with AtomicUsize for thread-safe access. - Introduce num_cpu() and set_num_cpu() functions for better encapsulation. - Update various modules to use the new atomic access methods for CPU count. - Refactor device tree and boot modules to utilize the new clock frequency management. - Enhance trap handler to use SpinLock for BOOT_TRAP_FRAME. - Improve error handling in syscall implementations by introducing copy_user_bytes and copy_user_array functions. - Update test macros to use SpinLock for thread-safe assertion recording. - Refactor Dentry full_path method to avoid unsafe pointer dereferencing. - Transition global file lock manager to lazy_static for better initialization. - Clean up unused zeroed variable assignments in character device file implementations. - Add AGENT.md for project maintenance guidelines and refactoring priorities.
There was a problem hiding this comment.
Code Review
This pull request refactors the kernel to replace unsafe mutable statics (such as NUM_CPU, CLOCK_FREQ, and test failure lists) with safe atomic types, getters/setters, and spinlocks. It also refactors user-space copying in system calls to use helper functions, removes redundant memory zeroing, and cleans up raw pointer usage in Dentry::full_path. However, several critical issues were identified in the review: a massive out-of-bounds write in task_struct.rs due to an incorrect count in write_bytes; an unsafe asynchronous access to a spinlock-protected BOOT_TRAP_FRAME in the LoongArch trap handler; potential Denial of Service (OOM) vulnerabilities in io.rs due to missing upper-bound checks on user-provided allocation sizes; a deadlock risk in futex_ops.rs from calling copy_from_user while holding a spinlock; and unidiomatic formatting in cpu.rs.
| unsafe { | ||
| // 清零整个 TrapFrame,避免旧值泄漏到用户态 | ||
| core::ptr::write_bytes(tf_ptr, 0, 1); | ||
| core::ptr::write_bytes(tf_ptr, 0, size_of::<TrapFrame>()); |
There was a problem hiding this comment.
The core::ptr::write_bytes function takes the count in terms of elements of type T (which is TrapFrame here), not bytes. Passing size_of::<TrapFrame>() will cause it to write size_of::<TrapFrame>() * size_of::<TrapFrame>() bytes, leading to a massive out-of-bounds write and critical kernel memory corruption. Revert this to 1 to correctly clear a single TrapFrame structure.
| core::ptr::write_bytes(tf_ptr, 0, size_of::<TrapFrame>()); | |
| core::ptr::write_bytes(tf_ptr, 0, 1); |
| let mut boot_trap_frame = BOOT_TRAP_FRAME.lock(); | ||
| // 设置内核栈指针用于用户态陷阱的栈切换 | ||
| let sp: usize; | ||
| core::arch::asm!("addi.d {0}, $sp, 0", out(reg) sp, options(nostack, preserves_flags)); | ||
| BOOT_TRAP_FRAME.kernel_sp = sp; | ||
| BOOT_TRAP_FRAME.cpu_ptr = crate::kernel::current_cpu() as *const _ as usize; | ||
| boot_trap_frame.kernel_sp = sp; | ||
| boot_trap_frame.cpu_ptr = crate::kernel::current_cpu() as *const _ as usize; | ||
|
|
||
| // KScratch0 <- TrapFrame 指针 | ||
| core::arch::asm!( | ||
| "csrwr {0}, 0x30", | ||
| in(reg) (&raw mut BOOT_TRAP_FRAME as *mut TrapFrame as usize), | ||
| in(reg) (&mut *boot_trap_frame as *mut TrapFrame as usize), | ||
| options(nostack, preserves_flags) | ||
| ); |
There was a problem hiding this comment.
Wrapping BOOT_TRAP_FRAME in a SpinLock is fundamentally unsafe because the lock is released when install_trap_entry returns, but the raw pointer to the inner TrapFrame is stored in KScratch0 and will be accessed asynchronously by the hardware/trap handler. To maintain consistency with the RISC-V architecture implementation, avoid using a temporary SpinLock here and instead manage the static lifetime safely as done in the RISC-V trap handler.
References
- Prioritize consistency with existing architecture implementations (like RISC-V) when adding support for a new architecture (like LoongArch).
| fn copy_user_bytes(src: *const u8, len: usize) -> Result<alloc::vec::Vec<u8>, isize> { | ||
| let mut buf = alloc::vec![0u8; len]; | ||
| unsafe { | ||
| crate::arch::ArchImpl::copy_from_user(UA::from_usize(src as usize), buf.as_mut_ptr(), len) | ||
| .map_err(|_| -(EFAULT as isize))?; | ||
| } | ||
| Ok(buf) | ||
| } |
There was a problem hiding this comment.
The len parameter is passed directly from user space (via count in write) without any upper-bound validation. Calling alloc::vec![0u8; len] with an untrusted, arbitrarily large len can easily exhaust kernel memory and trigger a kernel panic (OOM), leading to a Denial of Service (DoS) vulnerability. Enforce a reasonable maximum limit (e.g., 8MB) to prevent OOM.
fn copy_user_bytes(src: *const u8, len: usize) -> Result<alloc::vec::Vec<u8>, isize> {
if len > 8 * 1024 * 1024 {
return Err(-(EFAULT as isize));
}
let mut buf = alloc::vec![0u8; len];
unsafe {
crate::arch::ArchImpl::copy_from_user(UA::from_usize(src as usize), buf.as_mut_ptr(), len)
.map_err(|_| -(EFAULT as isize))?;
}
Ok(buf)
}| fn copy_user_array<T: Copy>( | ||
| src: *const T, | ||
| len: usize, | ||
| empty: T, | ||
| ) -> Result<alloc::vec::Vec<T>, isize> { | ||
| let mut buf = alloc::vec![empty; len]; | ||
| let size = len | ||
| .checked_mul(core::mem::size_of::<T>()) | ||
| .ok_or(-(EFAULT as isize))?; | ||
| unsafe { | ||
| crate::arch::ArchImpl::copy_from_user( | ||
| UA::from_usize(src as usize), | ||
| buf.as_mut_ptr() as *mut u8, | ||
| size, | ||
| ) | ||
| .map_err(|_| -(EFAULT as isize))?; | ||
| } | ||
| Ok(buf) | ||
| } |
There was a problem hiding this comment.
In copy_user_array, the overflow check len.checked_mul(core::mem::size_of::<T>()) is performed after allocating the vector alloc::vec![empty; len]. If len is extremely large or causes an overflow, the allocation will already have been attempted and failed/panicked. Additionally, there is no upper-bound limit on the allocation size, leading to the same OOM vulnerability as copy_user_bytes. Perform the overflow check and size limit check before allocating the vector.
fn copy_user_array<T: Copy>(
src: *const T,
len: usize,
empty: T,
) -> Result<alloc::vec::Vec<T>, isize> {
let size = len
.checked_mul(core::mem::size_of::<T>())
.ok_or(-(EFAULT as isize))?;
if size > 8 * 1024 * 1024 {
return Err(-(EFAULT as isize));
}
let mut buf = alloc::vec![empty; len];
unsafe {
crate::arch::ArchImpl::copy_from_user(
UA::from_usize(src as usize),
buf.as_mut_ptr() as *mut u8,
size,
)
.map_err(|_| -(EFAULT as isize))?;
}
Ok(buf)
}| let user_val = { | ||
| let mut val = core::mem::MaybeUninit::<u32>::uninit(); | ||
| let copy_result = unsafe { | ||
| crate::arch::ArchImpl::copy_from_user( | ||
| UA::from_usize(uaddr as usize), | ||
| val.as_mut_ptr() as *mut u8, | ||
| core::mem::size_of::<u32>(), | ||
| ) | ||
| }; | ||
| if copy_result.is_err() { | ||
| return -EFAULT; | ||
| } | ||
| unsafe { val.assume_init() } | ||
| }; |
There was a problem hiding this comment.
Calling copy_from_user (which can trigger a page fault) while holding the FUTEX_MANAGER spinlock is highly dangerous. If a page fault occurs, the fault handler may block or yield, which is illegal while holding a spinlock and will lead to deadlocks or kernel panics. To resolve this, you should read the user value before acquiring the FUTEX_MANAGER lock, or ensure that the lock is a sleepable mutex rather than a spinlock.
| #[rustfmt::skip] | ||
| pub fn current_cpu() -> &'static | ||
| mut Cpu { |
There was a problem hiding this comment.
Refactor kernel CPU and clock frequency management
closed #244
closed #245