rust: chrdev: fix potential use-after-free

The kernel's `struct cdev` is a reference-counted `kobject`. This
means that the object isn't guaranteed to be cleaned up after a
call to `cdev_del` - the cleanup may occur later.

Rust's `chrdev` places the `struct cdev` in memory owned by the
module. On module unload, it calls `cdev_del` and releases all
module memory, including the `struct cdev`. But that structure
might only be cleaned up later - resulting in a potential use-after-
free.

This issue is reliably triggered using CONFIG_DEBUG_KOBJECT_RELEASE,
which has been developed specifically to catch this subtle class of
bugs.

Fix by allocating the `cdev` using `cdev_alloc`, which stores the
object on the kernel's `kalloc` heap. Now it can outlive the
module, and be cleaned up+released when the kernel decides it's time.

Signed-off-by: Sven Van Asbroeck <[email protected]>

Author: Sven Van Asbroeck

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,69 @@ use crate::error::{Error, KernelResult};
 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,
+    ) -> KernelResult<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) -> KernelResult<()> {
+        // SAFETY: by the type invariants:
+        // - [`self.0`] is valid and non-null.
+        // - [`(*self.0).ops`] is valid, non-null, and has a static lifetime.
+        // - [`(*self.0).owner`] is valid and, if non-null, has module lifetime.
+        // This guarantees that [`(*self.0).ops`] and [`(*self.0).owner`] (if non-null)
+        // will live at least as long as [`self.0`].
+        // dev and count are scalars.
+        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 +154,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 +167,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 +199,9 @@ 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`.
+            // SAFETY: TODO needs explanation why unregister_chrdev_region
+            // is safe here.
             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());
             }
         }