From 52db5512392da69230928f2b00516737aa184665 Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Sun, 18 Apr 2021 20:26:54 -0400 Subject: [PATCH 1/2] rust: chrdev: fix use-after-free on module unload Note that this issue is potentially present on any driver module which stores its `cdev` in `kmalloc`-ed memory. This is not seen as a problem, as module unloading is currently "best effort" only. 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 `kmalloc`-ed memory. On module unload, it calls `cdev_del` and `kfree`s 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 --- rust/kernel/chrdev.rs | 98 ++++++++++++++++++++++++++++++++----------- 1 file changed, 74 insertions(+), 24 deletions(-) diff --git a/rust/kernel/chrdev.rs b/rust/kernel/chrdev.rs index 6ebaeac01b783f..1768b1ec01c75d 100644 --- a/rust/kernel/chrdev.rs +++ b/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 { + // 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 { dev: bindings::dev_t, used: usize, - cdevs: [MaybeUninit; N], + cdevs: [Option; N], _pin: PhantomPinned, } @@ -96,10 +152,11 @@ impl Registration<{ N }> { if res != 0 { return Err(Error::from_kernel_errno(res)); } + const NONE: Option = None; this.inner = Some(RegistrationInner { dev, used: 0, - cdevs: [MaybeUninit::::uninit(); N], + cdevs: [NONE; N], _pin: PhantomPinned, }); } @@ -108,22 +165,13 @@ impl 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::::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::::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 Sync for Registration<{ N }> {} impl 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()); } } From 2bcc169117fd28de31aa45b2e1a4b9ef86e6cb0f Mon Sep 17 00:00:00 2001 From: Sven Van Asbroeck Date: Sun, 2 May 2021 17:06:19 -0400 Subject: [PATCH 2/2] CI: fail workflow if kernel log contains a WARNING Pull Requests should not introduce any kernel warnings. Fail the workflow if warnings are spotted in the log. Signed-off-by: Sven Van Asbroeck --- .github/workflows/ci.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index c0e74541ee49a1..7b5ee5ebc92f77 100644 --- a/.github/workflows/ci.yaml +++ b/.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