VirtAddr::read_mut: return ReadErr instead of None

Andy-Python-Programmer · Andy-Python-Programmer · commit 49feaa0fc463 · 2023-03-15T17:37:51.000+11:00
This makes error handling in the syscalls much cleaner. Another thing
that we could do is specifically have a `UserVirtAddr` structure, which
on reads validates that the memory that we are reading is valid for
memory R/W (depending of mutablity) in the read_{mut} function and the
address is a valid userland address (under arch::userland_max_address()).

Signed-off-by: Andy-Python-Programmer &lt;andypythonappdeveloper@gmail.com&gt;
diff --git a/src/aero_kernel/src/drivers/e1000.rs b/src/aero_kernel/src/drivers/e1000.rs
@@ -41,7 +41,13 @@ enum Error {
     UnknownBar,
     NoEeprom,
     OutOfMemory,
-    NotSupported,
+    ReadErr(ReadErr),
+}
+
+impl From<ReadErr> for Error {
+    fn from(value: ReadErr) -> Self {
+        Self::ReadErr(value)
+    }
 }
 
 #[derive(Copy, Clone)]
@@ -446,9 +452,7 @@ impl E1000 {
         let phys = frame.start_address();
         let addr = phys.as_hhdm_virt();
 
-        let descriptors = addr
-            .read_mut::<[TxDescriptor; TX_DESC_NUM as usize]>()
-            .ok_or(Error::NotSupported)?;
+        let descriptors = addr.read_mut::<[TxDescriptor; TX_DESC_NUM as usize]>()?;
 
         for desc in descriptors {
             *desc = TxDescriptor::default();
@@ -485,9 +489,7 @@ impl E1000 {
         let phys = frame.start_address();
         let addr = phys.as_hhdm_virt();
 
-        let descriptors = addr
-            .read_mut::<[RxDescriptor; RX_DESC_NUM as usize]>()
-            .ok_or(Error::NotSupported)?;
+        let descriptors = addr.read_mut::<[RxDescriptor; RX_DESC_NUM as usize]>()?;
 
         for desc in descriptors {
             let frame: PhysFrame<Size4KiB> =
diff --git a/src/aero_kernel/src/drivers/pty.rs b/src/aero_kernel/src/drivers/pty.rs
@@ -108,18 +108,12 @@ impl INodeInterface for Master {
     fn ioctl(&self, command: usize, arg: usize) -> fs::Result<usize> {
         match command {
             TIOCGPTN => {
-                let id = VirtAddr::new(arg as u64)
-                    .read_mut::<u32>()
-                    .ok_or(FileSystemError::NotSupported)?;
-
+                let id = VirtAddr::new(arg as u64).read_mut::<u32>()?;
                 *id = self.id;
             }
 
             aero_syscall::TIOCSWINSZ => {
-                let winsize = VirtAddr::new(arg as u64)
-                    .read_mut::<WinSize>()
-                    .ok_or(FileSystemError::NotSupported)?;
-
+                let winsize = VirtAddr::new(arg as u64).read_mut::<WinSize>()?;
                 *self.window_size.lock_irq() = *winsize;
             }
 
@@ -180,29 +174,23 @@ impl INodeInterface for Slave {
 
         match command {
             aero_syscall::TIOCGWINSZ => {
-                let winsize = VirtAddr::new(arg as u64)
-                    .read_mut::<WinSize>()
-                    .ok_or(FileSystemError::NotSupported)?;
-
+                let winsize = VirtAddr::new(arg as u64).read_mut::<WinSize>()?;
                 *winsize = *self.master.window_size.lock_irq();
+
                 Ok(0)
             }
 
             aero_syscall::TCGETS => {
-                let termios = VirtAddr::new(arg as u64)
-                    .read_mut::<Termios>()
-                    .ok_or(FileSystemError::NotSupported)?;
-
+                let termios = VirtAddr::new(arg as u64).read_mut::<Termios>()?;
                 *termios = inner.termios;
+
                 Ok(0)
             }
 
             aero_syscall::TCSETSF => {
-                let termios = VirtAddr::new(arg as u64)
-                    .read_mut::<Termios>()
-                    .ok_or(FileSystemError::NotSupported)?;
-
+                let termios = VirtAddr::new(arg as u64).read_mut::<Termios>()?;
                 inner.termios = *termios;
+
                 Ok(0)
             }
 
diff --git a/src/aero_kernel/src/fs/devfs.rs b/src/aero_kernel/src/fs/devfs.rs
@@ -390,10 +390,7 @@ impl INodeInterface for DevFb {
             // Device independent colormap information can be get and set using
             // the `FBIOGETCMAP` and `FBIOPUTCMAP` ioctls.
             FBIOPUTCMAP => {
-                let struc = VirtAddr::new(arg as _)
-                    .read_mut::<FramebufferCmap>()
-                    .ok_or(FileSystemError::NotSupported);
-
+                let struc = VirtAddr::new(arg as _).read_mut::<FramebufferCmap>()?;
                 log::debug!("fbdev: `FBIOPUTCMAP` is a stub! {struc:?}");
                 Ok(0)
             }
diff --git a/src/aero_kernel/src/main.rs b/src/aero_kernel/src/main.rs
@@ -44,7 +44,8 @@
     nonnull_slice_from_raw_parts,
     maybe_uninit_write_slice,
     slice_ptr_get,
-    maybe_uninit_as_bytes
+    maybe_uninit_as_bytes,
+    pointer_is_aligned
 )]
 #![deny(trivial_numeric_casts, unused_allocation)]
 #![test_runner(crate::tests::test_runner)]
diff --git a/src/aero_kernel/src/mem/paging/addr.rs b/src/aero_kernel/src/mem/paging/addr.rs
@@ -23,6 +23,8 @@ use core::fmt;
 use core::iter::Step;
 use core::ops::{Add, AddAssign, Sub, SubAssign};
 
+use crate::fs::FileSystemError;
+
 use super::page_table::{PageOffset, PageTableIndex};
 use super::{PageSize, Size4KiB, VmFrame};
 
@@ -52,14 +54,28 @@ pub struct VirtAddr(u64);
 #[repr(transparent)]
 pub struct PhysAddr(u64);
 
-/// A passed `u64` was not a valid virtual address.
-///
-/// This means that bits 48 to 64 are not
-/// a valid sign extension and are not null either. So automatic sign extension would have
-/// overwritten possibly meaningful bits. This likely indicates a bug, for example an invalid
-/// address calculation.
-#[derive(Debug)]
-pub struct VirtAddrNotValid(u64);
+#[derive(Copy, Clone, Debug)]
+pub enum ReadErr {
+    Null,
+    NotAligned,
+}
+
+impl From<ReadErr> for FileSystemError {
+    fn from(_: ReadErr) -> Self {
+        // `FileSystemError::NotSupported` will be converted to `EINVAL` on
+        // syscall error conversion.
+        FileSystemError::NotSupported
+    }
+}
+
+impl From<ReadErr> for aero_syscall::SyscallError {
+    fn from(value: ReadErr) -> Self {
+        match value {
+            ReadErr::Null => Self::EINVAL,
+            ReadErr::NotAligned => Self::EACCES,
+        }
+    }
+}
 
 impl VirtAddr {
     /// Creates a new canonical virtual address.
@@ -99,20 +115,15 @@ impl VirtAddr {
     ///
     /// ## Example
     /// ```no_run
-    /// let address: &mut SomeStruct = VirtAddr::new(0xcafebabe)
-    ///    .read_mut::<SomeStruct>();
-    ///    .ok_or(AeroSyscallError::EFAULT)?;
+    /// let address: &mut SomeStruct = VirtAddr::new(0xcafebabe).read_mut::<SomeStruct>()?;
     /// ```
-    pub fn read_mut<'struc, T: Sized>(&self) -> Option<&'struc mut T> {
-        if self.validate_read::<T>() {
-            Some(unsafe { &mut *(self.as_mut_ptr() as *mut T) })
-        } else {
-            None
-        }
+    pub fn read_mut<'a, T: Sized>(&self) -> Result<&'a mut T, ReadErr> {
+        self.validate_read::<T>()?;
+        Ok(unsafe { &mut *(self.as_mut_ptr() as *mut T) })
     }
 
     pub fn as_bytes_mut(&self, size_bytes: usize) -> &mut [u8] {
-        assert!(self.validate_read::<&[u8]>());
+        self.validate_read::<&[u8]>().unwrap();
         unsafe { core::slice::from_raw_parts_mut(self.as_mut_ptr(), size_bytes) }
     }
 
@@ -122,10 +133,17 @@ impl VirtAddr {
     }
 
     /// Returns if the address is valid to read `sizeof(T)` bytes at the address.
-    fn validate_read<T: Sized>(&self) -> bool {
+    fn validate_read<T: Sized>(&self) -> Result<(), ReadErr> {
         // FIXME: (*self + core::mem::size_of::<T>()) <= crate::arch::task::userland_last_address()
-        // // in-range
-        self.0 != 0 // non-null
+        let raw = self.as_ptr::<T>();
+
+        if raw.is_null() {
+            return Err(ReadErr::Null);
+        } else if !raw.is_aligned() {
+            return Err(ReadErr::NotAligned);
+        }
+
+        Ok(())
     }
 
     /// Aligns the virtual address downwards to the given alignment.
diff --git a/src/aero_kernel/src/socket/inet.rs b/src/aero_kernel/src/socket/inet.rs
@@ -231,9 +231,7 @@ impl INodeInterface for InetSocket {
     fn ioctl(&self, command: usize, arg: usize) -> fs::Result<usize> {
         match command {
             SIOCGIFINDEX => {
-                let ifreq = VirtAddr::new(arg as _)
-                    .read_mut::<IfReq>()
-                    .ok_or(FileSystemError::NotSupported)?;
+                let ifreq = VirtAddr::new(arg as _).read_mut::<IfReq>()?;
 
                 let name = ifreq.name().unwrap();
                 assert!(name == "eth0");
@@ -243,9 +241,7 @@ impl INodeInterface for InetSocket {
             }
 
             SIOCGIFHWADDR => {
-                let ifreq = VirtAddr::new(arg as _)
-                    .read_mut::<IfReq>()
-                    .ok_or(FileSystemError::NotSupported)?;
+                let ifreq = VirtAddr::new(arg as _).read_mut::<IfReq>()?;
 
                 let name = ifreq.name().ok_or(FileSystemError::InvalidPath)?;
                 assert!(name == "eth0");
@@ -263,9 +259,7 @@ impl INodeInterface for InetSocket {
             }
 
             SIOCSIFADDR => {
-                let ifreq = VirtAddr::new(arg as _)
-                    .read_mut::<IfReq>()
-                    .ok_or(FileSystemError::NotSupported)?;
+                let ifreq = VirtAddr::new(arg as _).read_mut::<IfReq>()?;
 
                 let family = unsafe { ifreq.data.addr.sa_family };
 
@@ -274,7 +268,7 @@ impl INodeInterface for InetSocket {
                     VirtAddr::new(&unsafe { ifreq.data.addr } as *const _ as _),
                     family,
                 )
-                .ok_or(FileSystemError::NotSupported)?
+                .map_err(|_| FileSystemError::NotSupported)?
                 .as_inet()
                 .ok_or(FileSystemError::NotSupported)?;
 
diff --git a/src/aero_kernel/src/socket/mod.rs b/src/aero_kernel/src/socket/mod.rs
@@ -31,12 +31,12 @@ pub enum SocketAddr<'a> {
 }
 
 impl<'a> SocketAddr<'a> {
-    pub fn from_family(address: VirtAddr, family: u32) -> Option<Self> {
+    pub fn from_family(address: VirtAddr, family: u32) -> Result<Self, SyscallError> {
         match family {
-            AF_UNIX => Some(SocketAddr::Unix(address.read_mut::<SocketAddrUnix>()?)),
-            AF_INET => Some(SocketAddr::INet(address.read_mut::<SocketAddrInet>()?)),
+            AF_UNIX => Ok(SocketAddr::Unix(address.read_mut::<SocketAddrUnix>()?)),
+            AF_INET => Ok(SocketAddr::INet(address.read_mut::<SocketAddrInet>()?)),
 
-            _ => None,
+            _ => Err(SyscallError::EINVAL),
         }
     }
 
diff --git a/src/aero_kernel/src/socket/unix.rs b/src/aero_kernel/src/socket/unix.rs
@@ -111,8 +111,7 @@ pub struct AcceptQueue {
 
 impl AcceptQueue {
     /// # Parameters
-    /// * `backlog`: The maximum number of pending connections that the
-    ///              queue can hold.
+    /// * `backlog`: The maximum number of pending connections that the queue can hold.
     pub fn new(backlog: usize) -> Self {
         Self {
             sockets: VecDeque::with_capacity(backlog),
@@ -370,9 +369,7 @@ impl INodeInterface for UnixSocket {
         }
 
         if let Some((address, length)) = address {
-            let address = address
-                .read_mut::<SocketAddrUnix>()
-                .ok_or(FileSystemError::NotSupported)?;
+            let address = address.read_mut::<SocketAddrUnix>()?;
 
             if let Some(paddr) = peer.inner.lock_irq().address.as_ref() {
                 *address = paddr.clone();
diff --git a/src/aero_kernel/src/syscall/fs.rs b/src/aero_kernel/src/syscall/fs.rs
@@ -654,7 +654,7 @@ pub fn poll(fds: &mut [PollFd], timeout: usize, sigmask: usize) -> Result<usize,
 
     // The timeout can be NULL.
     let timeout = if timeout != 0x00 {
-        Some(crate::utils::validate_ptr(timeout as *const TimeSpec).ok_or(SyscallError::EINVAL)?)
+        Some(crate::utils::validate_ptr(timeout as *const TimeSpec)?)
     } else {
         None
     };
diff --git a/src/aero_kernel/src/syscall/futex.rs b/src/aero_kernel/src/syscall/futex.rs
@@ -89,7 +89,7 @@ impl FutexContainer {
         Self::validate_futex_ptr(uaddr)?;
 
         let key = Self::addr_as_futex_key(uaddr).ok_or(SyscallError::EINVAL)?;
-        let value = uaddr.read_mut::<AtomicU32>().ok_or(SyscallError::EINVAL)?;
+        let value = uaddr.read_mut::<AtomicU32>()?;
 
         if value.load(Ordering::SeqCst) == expected {
             let futex = self.get_alloc(key);
diff --git a/src/aero_kernel/src/syscall/net.rs b/src/aero_kernel/src/syscall/net.rs
@@ -18,12 +18,8 @@ use crate::userland::scheduler;
 /// Creates a [`SocketAddr`] from the provided userland socket structure address. This
 /// is done by looking at the family field present in every socket address structure.
 fn socket_addr_from_addr<'sys>(address: VirtAddr) -> Result<SocketAddr<'sys>, SyscallError> {
-    let family = address
-        .read_mut::<u32>()
-        .ok_or(SyscallError::EINVAL)?
-        .clone();
-
-    Ok(SocketAddr::from_family(address, family).ok_or(SyscallError::EINVAL)?)
+    let family = address.read_mut::<u32>()?.clone();
+    Ok(SocketAddr::from_family(address, family)?)
 }
 
 /// Connects the socket to the specified address.
@@ -49,9 +45,7 @@ pub fn accept(fd: usize, address: usize, length: usize) -> Result<usize, Syscall
     let address = if address != 0 && length != 0 {
         Some((
             VirtAddr::new(address as u64),
-            VirtAddr::new(length as u64)
-                .read_mut::<u32>()
-                .ok_or(SyscallError::EACCES)?,
+            VirtAddr::new(length as u64).read_mut::<u32>()?,
         ))
     } else {
         None
diff --git a/src/aero_kernel/src/utils/mod.rs b/src/aero_kernel/src/utils/mod.rs
@@ -20,7 +20,7 @@
 use alloc::{alloc::alloc_zeroed, sync::Arc};
 use core::{alloc::Layout, any::Any, cell::UnsafeCell, mem, ptr::Unique};
 
-use crate::mem::paging::{align_down, VirtAddr};
+use crate::mem::paging::{align_down, ReadErr, VirtAddr};
 
 #[cfg(target_arch = "x86_64")]
 use crate::arch::apic::get_cpu_count;
@@ -35,40 +35,42 @@ pub mod buffer;
 pub mod dma;
 pub mod sync;
 
-pub fn validate_mut_ptr<T>(ptr: *mut T) -> Option<&'static mut T> {
+pub fn validate_mut_ptr<T>(ptr: *mut T) -> Result<&'static mut T, ReadErr> {
     VirtAddr::new(ptr as _).read_mut::<T>()
 }
 
-pub fn validate_ptr<T>(ptr: *const T) -> Option<&'static T> {
+pub fn validate_ptr<T>(ptr: *const T) -> Result<&'static T, ReadErr> {
     // SAFETY: Safe to cast const pointer to mutable since the pointer is not
-    // mutated and the returned reference is immutable.
+    //         mutated and the returned reference is immutable.
     validate_mut_ptr(ptr as *mut T).map(|e| &*e)
 }
 
-pub fn validate_slice_mut<T>(ptr: *mut T, len: usize) -> Option<&'static mut [T]> {
+pub fn validate_slice_mut<T>(ptr: *mut T, len: usize) -> Result<&'static mut [T], ReadErr> {
     if len == 0 {
-        Some(&mut [])
+        Ok(&mut [])
     } else {
         let _ = validate_ptr(ptr)?; // ensure non-null and in-range
         let _ = validate_ptr(unsafe { ptr.add(len) })?; // ensure in-range
 
         // SAFETY: We have validated the pointer above.
-        Some(unsafe { core::slice::from_raw_parts_mut(ptr, len) })
+        Ok(unsafe { core::slice::from_raw_parts_mut(ptr, len) })
     }
 }
 
-pub fn validate_slice<T>(ptr: *const T, len: usize) -> Option<&'static [T]> {
+pub fn validate_slice<T>(ptr: *const T, len: usize) -> Result<&'static [T], ReadErr> {
     // SAFETY: Safe to cast const pointer to mutable since the pointer is not
-    // mutated and the returned reference is immutable.
+    //         mutated and the returned reference is immutable.
     validate_slice_mut(ptr as *mut T, len).map(|e| &*e)
 }
 
-pub fn validate_str(ptr: *const u8, len: usize) -> Option<&'static str> {
+pub fn validate_str(ptr: *const u8, len: usize) -> Result<&'static str, ReadErr> {
     let slice = validate_slice(ptr, len)?;
-    core::str::from_utf8(slice).ok()
+    core::str::from_utf8(slice).map_err(|_| ReadErr::Null)
 }
 
-pub fn validate_array_mut<T, const COUNT: usize>(ptr: *mut T) -> Option<&'static mut [T; COUNT]> {
+pub fn validate_array_mut<T, const COUNT: usize>(
+    ptr: *mut T,
+) -> Result<&'static mut [T; COUNT], ReadErr> {
     let slice = validate_slice_mut::<T>(ptr, COUNT);
     // Convert the validated slice to an array.
     //
diff --git a/src/aero_proc/src/syscall.rs b/src/aero_proc/src/syscall.rs

Original file line number	Diff line number	Diff line change
`@@ -31,12 +31,12 @@ pub enum SocketAddr<'a> {`
`31`	`31`	`}`
`32`	`32`
`33`	`33`	`impl<'a> SocketAddr<'a> {`
`34`		`- pub fn from_family(address: VirtAddr, family: u32) -> Option<Self> {`
	`34`	`+ pub fn from_family(address: VirtAddr, family: u32) -> Result<Self, SyscallError> {`
`35`	`35`	`match family {`
`36`		`- AF_UNIX => Some(SocketAddr::Unix(address.read_mut::<SocketAddrUnix>()?)),`
`37`		`- AF_INET => Some(SocketAddr::INet(address.read_mut::<SocketAddrInet>()?)),`
	`36`	`+ AF_UNIX => Ok(SocketAddr::Unix(address.read_mut::<SocketAddrUnix>()?)),`
	`37`	`+ AF_INET => Ok(SocketAddr::INet(address.read_mut::<SocketAddrInet>()?)),`
`38`	`38`
`39`		`- _ => None,`
	`39`	`+ _ => Err(SyscallError::EINVAL),`
`40`	`40`	`}`
`41`	`41`	`}`
`42`	`42`