Skip to content

Commit

Permalink
rust: driver: address soundness issue in RegistrationOps
Browse files Browse the repository at this point in the history
The `RegistrationOps` trait holds some obligations to the caller and
implementers. While being documented, the trait and the corresponding
functions haven't been marked as unsafe.

Hence, markt the trait and functions unsafe and add the corresponding
safety comments.

This patch does not include any fuctional changes.

Reported-by: Gary Guo <gary@garyguo.net>
Closes: https://lore.kernel.org/rust-for-linux/20241224195821.3b43302b.gary@garyguo.net/
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
  • Loading branch information
Danilo Krummrich committed Jan 14, 2025
1 parent 5a1f97e commit 3f1a25e
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 11 deletions.
25 changes: 20 additions & 5 deletions rust/kernel/driver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,23 +17,35 @@ use macros::{pin_data, pinned_drop};
/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
/// `bindings::__pci_register_driver` from `RegistrationOps::register` and
/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
pub trait RegistrationOps {
///
/// # Safety
///
/// A call to [`RegistrationOps::unregister`] for a given instance of `RegType` is only valid if a
/// preceding call to [`RegistrationOps::register`] has been successful.
pub unsafe trait RegistrationOps {
/// The type that holds information about the registration. This is typically a struct defined
/// by the C portion of the kernel.
type RegType: Default;

/// Registers a driver.
///
/// # Safety
///
/// On success, `reg` must remain pinned and valid until the matching call to
/// [`RegistrationOps::unregister`].
fn register(
unsafe fn register(
reg: &Opaque<Self::RegType>,
name: &'static CStr,
module: &'static ThisModule,
) -> Result;

/// Unregisters a driver previously registered with [`RegistrationOps::register`].
fn unregister(reg: &Opaque<Self::RegType>);
///
/// # Safety
///
/// Must only be called after a preceding successful call to [`RegistrationOps::register`] for
/// the same `reg`.
unsafe fn unregister(reg: &Opaque<Self::RegType>);
}

/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
Expand Down Expand Up @@ -68,7 +80,8 @@ impl<T: RegistrationOps> Registration<T> {
// just been initialised above, so it's also valid for read.
let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };

T::register(drv, name, module)
// SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
unsafe { T::register(drv, name, module) }
}),
})
}
Expand All @@ -77,7 +90,9 @@ impl<T: RegistrationOps> Registration<T> {
#[pinned_drop]
impl<T: RegistrationOps> PinnedDrop for Registration<T> {
fn drop(self: Pin<&mut Self>) {
T::unregister(&self.reg);
// SAFETY: The existence of `self` guarantees that `self.reg` has previously been
// successfully registered with `T::register`
unsafe { T::unregister(&self.reg) };
}
}

Expand Down
8 changes: 5 additions & 3 deletions rust/kernel/pci.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,12 @@ use kernel::prelude::*;
/// An adapter for the registration of PCI drivers.
pub struct Adapter<T: Driver>(T);

impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
// a preceding call to `register` has been successful.
unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
type RegType = bindings::pci_driver;

fn register(
unsafe fn register(
pdrv: &Opaque<Self::RegType>,
name: &'static CStr,
module: &'static ThisModule,
Expand All @@ -45,7 +47,7 @@ impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
})
}

fn unregister(pdrv: &Opaque<Self::RegType>) {
unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
// SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
unsafe { bindings::pci_unregister_driver(pdrv.get()) }
}
Expand Down
8 changes: 5 additions & 3 deletions rust/kernel/platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,12 @@ use core::ptr::addr_of_mut;
/// An adapter for the registration of platform drivers.
pub struct Adapter<T: Driver>(T);

impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
// a preceding call to `register` has been successful.
unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
type RegType = bindings::platform_driver;

fn register(
unsafe fn register(
pdrv: &Opaque<Self::RegType>,
name: &'static CStr,
module: &'static ThisModule,
Expand All @@ -44,7 +46,7 @@ impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
to_result(unsafe { bindings::__platform_driver_register(pdrv.get(), module.0) })
}

fn unregister(pdrv: &Opaque<Self::RegType>) {
unsafe fn unregister(pdrv: &Opaque<Self::RegType>) {
// SAFETY: `pdrv` is guaranteed to be a valid `RegType`.
unsafe { bindings::platform_driver_unregister(pdrv.get()) };
}
Expand Down

0 comments on commit 3f1a25e

Please sign in to comment.