From 460234a18b26edca16b19c6bb197cf3c96317c8d Mon Sep 17 00:00:00 2001 From: Zhang Junyang Date: Fri, 26 Jul 2024 22:45:05 +0800 Subject: [PATCH] Check userspace NULL pointers without triggering kernel page faults --- kernel/aster-nix/src/syscall/read.rs | 20 +++++-- kernel/aster-nix/src/syscall/write.rs | 16 ++++-- kernel/aster-nix/src/util/mod.rs | 78 ++++++++++++++++++++++----- kernel/aster-nix/src/vm/vmar/mod.rs | 2 +- 4 files changed, 96 insertions(+), 20 deletions(-) diff --git a/kernel/aster-nix/src/syscall/read.rs b/kernel/aster-nix/src/syscall/read.rs index 807937865..062d536e7 100644 --- a/kernel/aster-nix/src/syscall/read.rs +++ b/kernel/aster-nix/src/syscall/read.rs @@ -1,5 +1,7 @@ // SPDX-License-Identifier: MPL-2.0 +use core::cmp::min; + use super::SyscallReturn; use crate::{fs::file_table::FileDesc, prelude::*, util::write_bytes_to_user}; @@ -15,8 +17,20 @@ pub fn sys_read(fd: FileDesc, user_buf_addr: Vaddr, buf_len: usize) -> Result, if + // the user specified an empty buffer, we should detect errors by checking + // the file discriptor. If no errors detected, return 0 successfully. + let read_len = if buf_len != 0 { + let mut read_buf = vec![0u8; buf_len]; + let read_len = file.read(&mut read_buf)?; + write_bytes_to_user( + user_buf_addr, + &mut VmReader::from(&read_buf[..min(read_len, buf_len)]), + )?; + read_len + } else { + file.read(&mut [])? + }; + Ok(SyscallReturn::Return(read_len as _)) } diff --git a/kernel/aster-nix/src/syscall/write.rs b/kernel/aster-nix/src/syscall/write.rs index a9aa38ac6..bcb5fb17f 100644 --- a/kernel/aster-nix/src/syscall/write.rs +++ b/kernel/aster-nix/src/syscall/write.rs @@ -20,9 +20,17 @@ pub fn sys_write(fd: FileDesc, user_buf_ptr: Vaddr, user_buf_len: usize) -> Resu file_table.get_file(fd)?.clone() }; - let mut buffer = vec![0u8; user_buf_len]; - read_bytes_from_user(user_buf_ptr, &mut VmWriter::from(buffer.as_mut_slice()))?; - debug!("write content = {:?}", buffer); - let write_len = file.write(&buffer)?; + // According to , if + // the user specified an empty buffer, we should detect errors by checking + // the file discriptor. If no errors detected, return 0 successfully. + let write_len = if user_buf_len != 0 { + let mut buffer = vec![0u8; user_buf_len]; + read_bytes_from_user(user_buf_ptr, &mut VmWriter::from(buffer.as_mut_slice()))?; + debug!("write content = {:?}", buffer); + file.write(&buffer)? + } else { + file.write(&[])? + }; + Ok(SyscallReturn::Return(write_len as _)) } diff --git a/kernel/aster-nix/src/util/mod.rs b/kernel/aster-nix/src/util/mod.rs index e1fa5bb0a..a9f4fbf5a 100644 --- a/kernel/aster-nix/src/util/mod.rs +++ b/kernel/aster-nix/src/util/mod.rs @@ -15,15 +15,25 @@ pub mod random; pub use iovec::{copy_iovs_from_user, IoVec}; -/// Reads bytes into the `dest` `VmWriter` -/// from the user space of the current process. +/// Reads bytes into the destination `VmWriter` from the user space of the +/// current process. /// -/// If the reading is completely successful, returns `Ok`. -/// Otherwise, returns `Err`. +/// If the reading is completely successful, returns `Ok`. Otherwise, it +/// returns `Err`. +/// +/// If the destination `VmWriter` (`dest`) is empty, this function still +/// checks if the current task and user space are available. If they are, +/// it returns `Ok`. /// /// TODO: this API can be discarded and replaced with the API of `VmReader` /// after replacing all related `buf` usages. pub fn read_bytes_from_user(src: Vaddr, dest: &mut VmWriter<'_>) -> Result<()> { + let copy_len = dest.avail(); + + if copy_len > 0 { + check_vaddr(src)?; + } + let current_task = current_task().ok_or(Error::with_message( Errno::EFAULT, "the current task is missing", @@ -32,16 +42,18 @@ pub fn read_bytes_from_user(src: Vaddr, dest: &mut VmWriter<'_>) -> Result<()> { Errno::EFAULT, "the user space is missing", ))?; - let copy_len = dest.avail(); let mut user_reader = user_space.vm_space().reader(src, copy_len)?; user_reader.read_fallible(dest).map_err(|err| err.0)?; Ok(()) } -/// Reads a value of `Pod` type -/// from the user space of the current process. +/// Reads a value typed `Pod` from the user space of the current process. pub fn read_val_from_user(src: Vaddr) -> Result { + if core::mem::size_of::() > 0 { + check_vaddr(src)?; + } + let current_task = current_task().ok_or(Error::with_message( Errno::EFAULT, "the current task is missing", @@ -57,15 +69,25 @@ pub fn read_val_from_user(src: Vaddr) -> Result { Ok(user_reader.read_val()?) } -/// Writes bytes from the `src` `VmReader` -/// to the user space of the current process. +/// Writes bytes from the source `VmReader` to the user space of the current +/// process. /// -/// If the writing is completely successful, returns `Ok`, -/// Otherwise, returns `Err`. +/// If the writing is completely successful, returns `Ok`. Otherwise, it +/// returns `Err`. +/// +/// If the source `VmReader` (`src`) is empty, this function still checks if +/// the current task and user space are available. If they are, it returns +/// `Ok`. /// /// TODO: this API can be discarded and replaced with the API of `VmWriter` /// after replacing all related `buf` usages. pub fn write_bytes_to_user(dest: Vaddr, src: &mut VmReader<'_, KernelSpace>) -> Result<()> { + let copy_len = src.remain(); + + if copy_len > 0 { + check_vaddr(dest)?; + } + let current_task = current_task().ok_or(Error::with_message( Errno::EFAULT, "the current task is missing", @@ -74,7 +96,6 @@ pub fn write_bytes_to_user(dest: Vaddr, src: &mut VmReader<'_, KernelSpace>) -> Errno::EFAULT, "the user space is missing", ))?; - let copy_len = src.remain(); let mut user_writer = user_space.vm_space().writer(dest, copy_len)?; user_writer.write_fallible(src).map_err(|err| err.0)?; @@ -83,6 +104,10 @@ pub fn write_bytes_to_user(dest: Vaddr, src: &mut VmReader<'_, KernelSpace>) -> /// Writes `val` to the user space of the current process. pub fn write_val_to_user(dest: Vaddr, val: &T) -> Result<()> { + if core::mem::size_of::() > 0 { + check_vaddr(dest)?; + } + let current_task = current_task().ok_or(Error::with_message( Errno::EFAULT, "the current task is missing", @@ -107,6 +132,10 @@ pub fn write_val_to_user(dest: Vaddr, val: &T) -> Result<()> { /// The original Linux implementation can be found at: /// pub fn read_cstring_from_user(addr: Vaddr, max_len: usize) -> Result { + if max_len > 0 { + check_vaddr(addr)?; + } + let current = current!(); let vmar = current.root_vmar(); read_cstring_from_vmar(vmar, addr, max_len) @@ -114,6 +143,10 @@ pub fn read_cstring_from_user(addr: Vaddr, max_len: usize) -> Result { /// Read CString from `vmar`. If possible, use `read_cstring_from_user` instead. pub fn read_cstring_from_vmar(vmar: &Vmar, addr: Vaddr, max_len: usize) -> Result { + if max_len > 0 { + check_vaddr(addr)?; + } + let mut buffer: Vec = Vec::with_capacity(max_len); let mut cur_addr = addr; @@ -175,3 +208,24 @@ const fn has_zero(value: usize) -> bool { value.wrapping_sub(ONE_BITS) & !value & HIGH_BITS != 0 } + +/// Check if the user space pointer is below the lowest userspace address. +/// +/// If a pointer is below the lowest userspace address, it is likely to be a +/// NULL pointer. Reading from or writing to a NULL pointer should trigger a +/// segmentation fault. +/// +/// If it is not checked here, a kernel page fault will happen and we would +/// deny the access in the page fault handler either. It may save a page fault +/// in some occasions. More importantly, double page faults may not be handled +/// quite well on some platforms. +fn check_vaddr(va: Vaddr) -> Result<()> { + if va < crate::vm::vmar::ROOT_VMAR_LOWEST_ADDR { + Err(Error::with_message( + Errno::EFAULT, + "Bad user space pointer specified", + )) + } else { + Ok(()) + } +} diff --git a/kernel/aster-nix/src/vm/vmar/mod.rs b/kernel/aster-nix/src/vm/vmar/mod.rs index eeb603705..36da4e230 100644 --- a/kernel/aster-nix/src/vm/vmar/mod.rs +++ b/kernel/aster-nix/src/vm/vmar/mod.rs @@ -123,7 +123,7 @@ impl VmarInner { } } -const ROOT_VMAR_LOWEST_ADDR: Vaddr = 0x001_0000; // 64 KiB is the Linux configurable default +pub const ROOT_VMAR_LOWEST_ADDR: Vaddr = 0x001_0000; // 64 KiB is the Linux configurable default const ROOT_VMAR_CAP_ADDR: Vaddr = MAX_USERSPACE_VADDR; impl Interval for Arc {