Check userspace NULL pointers without triggering kernel page faults

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<Sy
         file_table.get_file(fd)?.clone()
     };
 
-    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.as_slice()))?;
+    // According to <https://man7.org/linux/man-pages/man2/read.2.html>, 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 _))
 }

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 <https://man7.org/linux/man-pages/man2/write.2.html>, 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 _))
 }

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<T: Pod>(src: Vaddr) -> Result<T> {
+    if core::mem::size_of::<T>() > 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<T: Pod>(src: Vaddr) -> Result<T> {
     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<T: Pod>(dest: Vaddr, val: &T) -> Result<()> {
+    if core::mem::size_of::<T>() > 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<T: Pod>(dest: Vaddr, val: &T) -> Result<()> {
 /// The original Linux implementation can be found at:
 /// <https://elixir.bootlin.com/linux/v6.0.9/source/lib/strncpy_from_user.c#L28>
 pub fn read_cstring_from_user(addr: Vaddr, max_len: usize) -> Result<CString> {
+    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<CString> {
 
 /// Read CString from `vmar`. If possible, use `read_cstring_from_user` instead.
 pub fn read_cstring_from_vmar(vmar: &Vmar<Full>, addr: Vaddr, max_len: usize) -> Result<CString> {
+    if max_len > 0 {
+        check_vaddr(addr)?;
+    }
+
     let mut buffer: Vec<u8> = 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(())
+    }
+}

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<usize> for Arc<Vmar_> {