Merge pull request #207 from TheSven73/rust-for-linux-cdev-uaf

RFC: rust: chrdev: fix use-after-free on module unload

Author: alex

.github/workflows/ci.yaml

@@ -252,6 +252,10 @@ jobs:
             | sed s:$'\r'$:: \
             | tee qemu-stdout.log
 
+      # The kernel should not be generating any warnings
+      - run: |
+          ! grep '] WARNING:' qemu-stdout.log
+
       # Check
       - run: |
           grep '] rust_minimal: Rust minimal sample (init)$' qemu-stdout.log

rust/kernel/chrdev.rs

@@ -11,7 +11,6 @@
 use alloc::boxed::Box;
 use core::convert::TryInto;
 use core::marker::PhantomPinned;
-use core::mem::MaybeUninit;
 use core::pin::Pin;
 
 use crate::bindings;
@@ -20,10 +19,67 @@ use crate::error::{Error, Result};
 use crate::file_operations;
 use crate::types::CStr;
 
+/// Character device.
+///
+/// # Invariants
+///
+/// - [`self.0`] is valid and non-null.
+/// - [`(*self.0).ops`] is valid, non-null and has static lifetime.
+/// - [`(*self.0).owner`] is valid and, if non-null, has module lifetime.
+struct Cdev(*mut bindings::cdev);
+
+impl Cdev {
+    fn alloc(
+        fops: &'static bindings::file_operations,
+        module: &'static crate::ThisModule,
+    ) -> Result<Self> {
+        // SAFETY: FFI call.
+        let cdev = unsafe { bindings::cdev_alloc() };
+        if cdev.is_null() {
+            return Err(Error::ENOMEM);
+        }
+        // SAFETY: `cdev` is valid and non-null since `cdev_alloc()`
+        // returned a valid pointer which was null-checked.
+        unsafe {
+            (*cdev).ops = fops;
+            (*cdev).owner = module.0;
+        }
+        // INVARIANTS:
+        // - [`self.0`] is valid and non-null.
+        // - [`(*self.0).ops`] is valid, non-null and has static lifetime,
+        //   because it was coerced from a reference with static lifetime.
+        // - [`(*self.0).owner`] is valid and, if non-null, has module lifetime,
+        //   guaranteed by the [`ThisModule`] invariant.
+        Ok(Self(cdev))
+    }
+
+    fn add(&mut self, dev: bindings::dev_t, count: c_types::c_uint) -> Result {
+        // SAFETY: according to the type invariants:
+        // - [`self.0`] can be safely passed to [`bindings::cdev_add`].
+        // - [`(*self.0).ops`] will live at least as long as [`self.0`].
+        // - [`(*self.0).owner`] will live at least as long as the
+        //   module, which is an implicit requirement.
+        let rc = unsafe { bindings::cdev_add(self.0, dev, count) };
+        if rc != 0 {
+            return Err(Error::from_kernel_errno(rc));
+        }
+        Ok(())
+    }
+}
+
+impl Drop for Cdev {
+    fn drop(&mut self) {
+        // SAFETY: [`self.0`] is valid and non-null by the type invariants.
+        unsafe {
+            bindings::cdev_del(self.0);
+        }
+    }
+}
+
 struct RegistrationInner<const N: usize> {
     dev: bindings::dev_t,
     used: usize,
-    cdevs: [MaybeUninit<bindings::cdev>; N],
+    cdevs: [Option<Cdev>; N],
     _pin: PhantomPinned,
 }
 
@@ -96,10 +152,11 @@ impl<const N: usize> Registration<{ N }> {
             if res != 0 {
                 return Err(Error::from_kernel_errno(res));
             }
+            const NONE: Option<Cdev> = None;
             this.inner = Some(RegistrationInner {
                 dev,
                 used: 0,
-                cdevs: [MaybeUninit::<bindings::cdev>::uninit(); N],
+                cdevs: [NONE; N],
                 _pin: PhantomPinned,
             });
         }
@@ -108,22 +165,13 @@ impl<const N: usize> Registration<{ N }> {
         if inner.used == N {
             return Err(Error::EINVAL);
         }
-        let cdev = inner.cdevs[inner.used].as_mut_ptr();
-        // SAFETY: Calling unsafe functions and manipulating `MaybeUninit`
-        // pointer.
-        unsafe {
-            bindings::cdev_init(
-                cdev,
-                // SAFETY: The adapter doesn't retrieve any state yet, so it's compatible with any
-                // registration.
-                file_operations::FileOperationsVtable::<Self, T>::build(),
-            );
-            (*cdev).owner = this.this_module.0;
-            let rc = bindings::cdev_add(cdev, inner.dev + inner.used as bindings::dev_t, 1);
-            if rc != 0 {
-                return Err(Error::from_kernel_errno(rc));
-            }
-        }
+
+        // SAFETY: The adapter doesn't retrieve any state yet, so it's compatible with any
+        // registration.
+        let fops = unsafe { file_operations::FileOperationsVtable::<Self, T>::build() };
+        let mut cdev = Cdev::alloc(fops, &this.this_module)?;
+        cdev.add(inner.dev + inner.used as bindings::dev_t, 1)?;
+        inner.cdevs[inner.used].replace(cdev);
         inner.used += 1;
         Ok(())
     }
@@ -149,12 +197,14 @@ unsafe impl<const N: usize> Sync for Registration<{ N }> {}
 impl<const N: usize> Drop for Registration<{ N }> {
     fn drop(&mut self) {
         if let Some(inner) = self.inner.as_mut() {
-            // SAFETY: Calling unsafe functions, `0..inner.used` of
-            // `inner.cdevs` are initialized in `Registration::register`.
+            // Replicate kernel C behaviour: drop [`Cdev`]s before calling
+            // [`bindings::unregister_chrdev_region`].
+            for i in 0..inner.used {
+                inner.cdevs[i].take();
+            }
+            // SAFETY: [`self.inner`] is Some, so [`inner.dev`] was previously
+            // created using [`bindings::alloc_chrdev_region`].
             unsafe {
-                for i in 0..inner.used {
-                    bindings::cdev_del(inner.cdevs[i].as_mut_ptr());
-                }
                 bindings::unregister_chrdev_region(inner.dev, N.try_into().unwrap());
             }
         }