Skip to content

Commit 5dbeeb2

Browse files
committed
Merge tag 'driver-core-6.19-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core
Pull driver core fixes from Danilo Krummrich: - Always inline I/O and IRQ methods using build_assert!() to avoid false positive build errors - Do not free the driver's device private data in I2C shutdown() avoiding race conditions that can lead to UAF bugs - Drop the driver's device private data after the driver has been fully unbound from its device to avoid UAF bugs from &Device<Bound> scopes, such as IRQ callbacks * tag 'driver-core-6.19-rc7' of git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core: rust: driver: drop device private data post unbind rust: driver: add DriverData type to the DriverLayout trait rust: driver: add DEVICE_DRIVER_OFFSET to the DriverLayout trait rust: driver: introduce a DriverLayout trait rust: auxiliary: add Driver::unbind() callback rust: i2c: do not drop device private data on shutdown() rust: irq: always inline functions using build_assert with arguments rust: io: always inline functions using build_assert with arguments
2 parents 12a0094 + a995fe1 commit 5dbeeb2

12 files changed

Lines changed: 209 additions & 74 deletions

File tree

drivers/base/dd.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -548,6 +548,8 @@ static DEVICE_ATTR_RW(state_synced);
548548
static void device_unbind_cleanup(struct device *dev)
549549
{
550550
devres_release_all(dev);
551+
if (dev->driver->p_cb.post_unbind_rust)
552+
dev->driver->p_cb.post_unbind_rust(dev);
551553
arch_teardown_dma_ops(dev);
552554
kfree(dev->dma_range_map);
553555
dev->dma_range_map = NULL;

include/linux/device/driver.h

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ enum probe_type {
8585
* uevent.
8686
* @p: Driver core's private data, no one other than the driver
8787
* core can touch this.
88+
* @p_cb: Callbacks private to the driver core; no one other than the
89+
* driver core is allowed to touch this.
8890
*
8991
* The device driver-model tracks all of the drivers known to the system.
9092
* The main reason for this tracking is to enable the driver core to match
@@ -119,6 +121,13 @@ struct device_driver {
119121
void (*coredump) (struct device *dev);
120122

121123
struct driver_private *p;
124+
struct {
125+
/*
126+
* Called after remove() and after all devres entries have been
127+
* processed. This is a Rust only callback.
128+
*/
129+
void (*post_unbind_rust)(struct device *dev);
130+
} p_cb;
122131
};
123132

124133

rust/kernel/auxiliary.rs

Lines changed: 33 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,22 @@ use core::{
2323
/// An adapter for the registration of auxiliary drivers.
2424
pub struct Adapter<T: Driver>(T);
2525

26-
// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
26+
// SAFETY:
27+
// - `bindings::auxiliary_driver` is a C type declared as `repr(C)`.
28+
// - `T` is the type of the driver's device private data.
29+
// - `struct auxiliary_driver` embeds a `struct device_driver`.
30+
// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
31+
unsafe impl<T: Driver + 'static> driver::DriverLayout for Adapter<T> {
32+
type DriverType = bindings::auxiliary_driver;
33+
type DriverData = T;
34+
const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
35+
}
36+
37+
// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
2738
// a preceding call to `register` has been successful.
2839
unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
29-
type RegType = bindings::auxiliary_driver;
30-
3140
unsafe fn register(
32-
adrv: &Opaque<Self::RegType>,
41+
adrv: &Opaque<Self::DriverType>,
3342
name: &'static CStr,
3443
module: &'static ThisModule,
3544
) -> Result {
@@ -41,14 +50,14 @@ unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
4150
(*adrv.get()).id_table = T::ID_TABLE.as_ptr();
4251
}
4352

44-
// SAFETY: `adrv` is guaranteed to be a valid `RegType`.
53+
// SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
4554
to_result(unsafe {
4655
bindings::__auxiliary_driver_register(adrv.get(), module.0, name.as_char_ptr())
4756
})
4857
}
4958

50-
unsafe fn unregister(adrv: &Opaque<Self::RegType>) {
51-
// SAFETY: `adrv` is guaranteed to be a valid `RegType`.
59+
unsafe fn unregister(adrv: &Opaque<Self::DriverType>) {
60+
// SAFETY: `adrv` is guaranteed to be a valid `DriverType`.
5261
unsafe { bindings::auxiliary_driver_unregister(adrv.get()) }
5362
}
5463
}
@@ -87,7 +96,9 @@ impl<T: Driver + 'static> Adapter<T> {
8796
// SAFETY: `remove_callback` is only ever called after a successful call to
8897
// `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
8998
// and stored a `Pin<KBox<T>>`.
90-
drop(unsafe { adev.as_ref().drvdata_obtain::<T>() });
99+
let data = unsafe { adev.as_ref().drvdata_borrow::<T>() };
100+
101+
T::unbind(adev, data);
91102
}
92103
}
93104

@@ -187,6 +198,20 @@ pub trait Driver {
187198
///
188199
/// Called when an auxiliary device is matches a corresponding driver.
189200
fn probe(dev: &Device<device::Core>, id_info: &Self::IdInfo) -> impl PinInit<Self, Error>;
201+
202+
/// Auxiliary driver unbind.
203+
///
204+
/// Called when a [`Device`] is unbound from its bound [`Driver`]. Implementing this callback
205+
/// is optional.
206+
///
207+
/// This callback serves as a place for drivers to perform teardown operations that require a
208+
/// `&Device<Core>` or `&Device<Bound>` reference. For instance, drivers may try to perform I/O
209+
/// operations to gracefully tear down the device.
210+
///
211+
/// Otherwise, release operations for driver resources should be performed in `Self::drop`.
212+
fn unbind(dev: &Device<device::Core>, this: Pin<&Self>) {
213+
let _ = (dev, this);
214+
}
190215
}
191216

192217
/// The auxiliary device representation.

rust/kernel/device.rs

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -232,30 +232,32 @@ impl Device<CoreInternal> {
232232
///
233233
/// # Safety
234234
///
235-
/// - Must only be called once after a preceding call to [`Device::set_drvdata`].
236235
/// - The type `T` must match the type of the `ForeignOwnable` previously stored by
237236
/// [`Device::set_drvdata`].
238-
pub unsafe fn drvdata_obtain<T: 'static>(&self) -> Pin<KBox<T>> {
237+
pub(crate) unsafe fn drvdata_obtain<T: 'static>(&self) -> Option<Pin<KBox<T>>> {
239238
// SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
240239
let ptr = unsafe { bindings::dev_get_drvdata(self.as_raw()) };
241240

242241
// SAFETY: By the type invariants, `self.as_raw()` is a valid pointer to a `struct device`.
243242
unsafe { bindings::dev_set_drvdata(self.as_raw(), core::ptr::null_mut()) };
244243

244+
if ptr.is_null() {
245+
return None;
246+
}
247+
245248
// SAFETY:
246-
// - By the safety requirements of this function, `ptr` comes from a previous call to
247-
// `into_foreign()`.
249+
// - If `ptr` is not NULL, it comes from a previous call to `into_foreign()`.
248250
// - `dev_get_drvdata()` guarantees to return the same pointer given to `dev_set_drvdata()`
249251
// in `into_foreign()`.
250-
unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) }
252+
Some(unsafe { Pin::<KBox<T>>::from_foreign(ptr.cast()) })
251253
}
252254

253255
/// Borrow the driver's private data bound to this [`Device`].
254256
///
255257
/// # Safety
256258
///
257-
/// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
258-
/// [`Device::drvdata_obtain`].
259+
/// - Must only be called after a preceding call to [`Device::set_drvdata`] and before the
260+
/// device is fully unbound.
259261
/// - The type `T` must match the type of the `ForeignOwnable` previously stored by
260262
/// [`Device::set_drvdata`].
261263
pub unsafe fn drvdata_borrow<T: 'static>(&self) -> Pin<&T> {
@@ -271,7 +273,7 @@ impl Device<Bound> {
271273
/// # Safety
272274
///
273275
/// - Must only be called after a preceding call to [`Device::set_drvdata`] and before
274-
/// [`Device::drvdata_obtain`].
276+
/// the device is fully unbound.
275277
/// - The type `T` must match the type of the `ForeignOwnable` previously stored by
276278
/// [`Device::set_drvdata`].
277279
unsafe fn drvdata_unchecked<T: 'static>(&self) -> Pin<&T> {
@@ -320,7 +322,7 @@ impl Device<Bound> {
320322

321323
// SAFETY:
322324
// - The above check of `dev_get_drvdata()` guarantees that we are called after
323-
// `set_drvdata()` and before `drvdata_obtain()`.
325+
// `set_drvdata()`.
324326
// - We've just checked that the type of the driver's private data is in fact `T`.
325327
Ok(unsafe { self.drvdata_unchecked() })
326328
}

rust/kernel/driver.rs

Lines changed: 70 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -99,31 +99,51 @@ use crate::{acpi, device, of, str::CStr, try_pin_init, types::Opaque, ThisModule
9999
use core::pin::Pin;
100100
use pin_init::{pin_data, pinned_drop, PinInit};
101101

102+
/// Trait describing the layout of a specific device driver.
103+
///
104+
/// This trait describes the layout of a specific driver structure, such as `struct pci_driver` or
105+
/// `struct platform_driver`.
106+
///
107+
/// # Safety
108+
///
109+
/// Implementors must guarantee that:
110+
/// - `DriverType` is `repr(C)`,
111+
/// - `DriverData` is the type of the driver's device private data.
112+
/// - `DriverType` embeds a valid `struct device_driver` at byte offset `DEVICE_DRIVER_OFFSET`.
113+
pub unsafe trait DriverLayout {
114+
/// The specific driver type embedding a `struct device_driver`.
115+
type DriverType: Default;
116+
117+
/// The type of the driver's device private data.
118+
type DriverData;
119+
120+
/// Byte offset of the embedded `struct device_driver` within `DriverType`.
121+
///
122+
/// This must correspond exactly to the location of the embedded `struct device_driver` field.
123+
const DEVICE_DRIVER_OFFSET: usize;
124+
}
125+
102126
/// The [`RegistrationOps`] trait serves as generic interface for subsystems (e.g., PCI, Platform,
103127
/// Amba, etc.) to provide the corresponding subsystem specific implementation to register /
104-
/// unregister a driver of the particular type (`RegType`).
128+
/// unregister a driver of the particular type (`DriverType`).
105129
///
106-
/// For instance, the PCI subsystem would set `RegType` to `bindings::pci_driver` and call
130+
/// For instance, the PCI subsystem would set `DriverType` to `bindings::pci_driver` and call
107131
/// `bindings::__pci_register_driver` from `RegistrationOps::register` and
108132
/// `bindings::pci_unregister_driver` from `RegistrationOps::unregister`.
109133
///
110134
/// # Safety
111135
///
112-
/// A call to [`RegistrationOps::unregister`] for a given instance of `RegType` is only valid if a
113-
/// preceding call to [`RegistrationOps::register`] has been successful.
114-
pub unsafe trait RegistrationOps {
115-
/// The type that holds information about the registration. This is typically a struct defined
116-
/// by the C portion of the kernel.
117-
type RegType: Default;
118-
136+
/// A call to [`RegistrationOps::unregister`] for a given instance of `DriverType` is only valid if
137+
/// a preceding call to [`RegistrationOps::register`] has been successful.
138+
pub unsafe trait RegistrationOps: DriverLayout {
119139
/// Registers a driver.
120140
///
121141
/// # Safety
122142
///
123143
/// On success, `reg` must remain pinned and valid until the matching call to
124144
/// [`RegistrationOps::unregister`].
125145
unsafe fn register(
126-
reg: &Opaque<Self::RegType>,
146+
reg: &Opaque<Self::DriverType>,
127147
name: &'static CStr,
128148
module: &'static ThisModule,
129149
) -> Result;
@@ -134,7 +154,7 @@ pub unsafe trait RegistrationOps {
134154
///
135155
/// Must only be called after a preceding successful call to [`RegistrationOps::register`] for
136156
/// the same `reg`.
137-
unsafe fn unregister(reg: &Opaque<Self::RegType>);
157+
unsafe fn unregister(reg: &Opaque<Self::DriverType>);
138158
}
139159

140160
/// A [`Registration`] is a generic type that represents the registration of some driver type (e.g.
@@ -146,7 +166,7 @@ pub unsafe trait RegistrationOps {
146166
#[pin_data(PinnedDrop)]
147167
pub struct Registration<T: RegistrationOps> {
148168
#[pin]
149-
reg: Opaque<T::RegType>,
169+
reg: Opaque<T::DriverType>,
150170
}
151171

152172
// SAFETY: `Registration` has no fields or methods accessible via `&Registration`, so it is safe to
@@ -157,17 +177,51 @@ unsafe impl<T: RegistrationOps> Sync for Registration<T> {}
157177
// any thread, so `Registration` is `Send`.
158178
unsafe impl<T: RegistrationOps> Send for Registration<T> {}
159179

160-
impl<T: RegistrationOps> Registration<T> {
180+
impl<T: RegistrationOps + 'static> Registration<T> {
181+
extern "C" fn post_unbind_callback(dev: *mut bindings::device) {
182+
// SAFETY: The driver core only ever calls the post unbind callback with a valid pointer to
183+
// a `struct device`.
184+
//
185+
// INVARIANT: `dev` is valid for the duration of the `post_unbind_callback()`.
186+
let dev = unsafe { &*dev.cast::<device::Device<device::CoreInternal>>() };
187+
188+
// `remove()` and all devres callbacks have been completed at this point, hence drop the
189+
// driver's device private data.
190+
//
191+
// SAFETY: By the safety requirements of the `Driver` trait, `T::DriverData` is the
192+
// driver's device private data type.
193+
drop(unsafe { dev.drvdata_obtain::<T::DriverData>() });
194+
}
195+
196+
/// Attach generic `struct device_driver` callbacks.
197+
fn callbacks_attach(drv: &Opaque<T::DriverType>) {
198+
let ptr = drv.get().cast::<u8>();
199+
200+
// SAFETY:
201+
// - `drv.get()` yields a valid pointer to `Self::DriverType`.
202+
// - Adding `DEVICE_DRIVER_OFFSET` yields the address of the embedded `struct device_driver`
203+
// as guaranteed by the safety requirements of the `Driver` trait.
204+
let base = unsafe { ptr.add(T::DEVICE_DRIVER_OFFSET) };
205+
206+
// CAST: `base` points to the offset of the embedded `struct device_driver`.
207+
let base = base.cast::<bindings::device_driver>();
208+
209+
// SAFETY: It is safe to set the fields of `struct device_driver` on initialization.
210+
unsafe { (*base).p_cb.post_unbind_rust = Some(Self::post_unbind_callback) };
211+
}
212+
161213
/// Creates a new instance of the registration object.
162214
pub fn new(name: &'static CStr, module: &'static ThisModule) -> impl PinInit<Self, Error> {
163215
try_pin_init!(Self {
164-
reg <- Opaque::try_ffi_init(|ptr: *mut T::RegType| {
216+
reg <- Opaque::try_ffi_init(|ptr: *mut T::DriverType| {
165217
// SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write.
166-
unsafe { ptr.write(T::RegType::default()) };
218+
unsafe { ptr.write(T::DriverType::default()) };
167219

168220
// SAFETY: `try_ffi_init` guarantees that `ptr` is valid for write, and it has
169221
// just been initialised above, so it's also valid for read.
170-
let drv = unsafe { &*(ptr as *const Opaque<T::RegType>) };
222+
let drv = unsafe { &*(ptr as *const Opaque<T::DriverType>) };
223+
224+
Self::callbacks_attach(drv);
171225

172226
// SAFETY: `drv` is guaranteed to be pinned until `T::unregister`.
173227
unsafe { T::register(drv, name, module) }

rust/kernel/i2c.rs

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,22 @@ macro_rules! i2c_device_table {
9292
/// An adapter for the registration of I2C drivers.
9393
pub struct Adapter<T: Driver>(T);
9494

95-
// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if
95+
// SAFETY:
96+
// - `bindings::i2c_driver` is a C type declared as `repr(C)`.
97+
// - `T` is the type of the driver's device private data.
98+
// - `struct i2c_driver` embeds a `struct device_driver`.
99+
// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`.
100+
unsafe impl<T: Driver + 'static> driver::DriverLayout for Adapter<T> {
101+
type DriverType = bindings::i2c_driver;
102+
type DriverData = T;
103+
const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver);
104+
}
105+
106+
// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if
96107
// a preceding call to `register` has been successful.
97108
unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
98-
type RegType = bindings::i2c_driver;
99-
100109
unsafe fn register(
101-
idrv: &Opaque<Self::RegType>,
110+
idrv: &Opaque<Self::DriverType>,
102111
name: &'static CStr,
103112
module: &'static ThisModule,
104113
) -> Result {
@@ -133,12 +142,12 @@ unsafe impl<T: Driver + 'static> driver::RegistrationOps for Adapter<T> {
133142
(*idrv.get()).driver.acpi_match_table = acpi_table;
134143
}
135144

136-
// SAFETY: `idrv` is guaranteed to be a valid `RegType`.
145+
// SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
137146
to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) })
138147
}
139148

140-
unsafe fn unregister(idrv: &Opaque<Self::RegType>) {
141-
// SAFETY: `idrv` is guaranteed to be a valid `RegType`.
149+
unsafe fn unregister(idrv: &Opaque<Self::DriverType>) {
150+
// SAFETY: `idrv` is guaranteed to be a valid `DriverType`.
142151
unsafe { bindings::i2c_del_driver(idrv.get()) }
143152
}
144153
}
@@ -169,9 +178,9 @@ impl<T: Driver + 'static> Adapter<T> {
169178
// SAFETY: `remove_callback` is only ever called after a successful call to
170179
// `probe_callback`, hence it's guaranteed that `I2cClient::set_drvdata()` has been called
171180
// and stored a `Pin<KBox<T>>`.
172-
let data = unsafe { idev.as_ref().drvdata_obtain::<T>() };
181+
let data = unsafe { idev.as_ref().drvdata_borrow::<T>() };
173182

174-
T::unbind(idev, data.as_ref());
183+
T::unbind(idev, data);
175184
}
176185

177186
extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
@@ -181,9 +190,9 @@ impl<T: Driver + 'static> Adapter<T> {
181190
// SAFETY: `shutdown_callback` is only ever called after a successful call to
182191
// `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called
183192
// and stored a `Pin<KBox<T>>`.
184-
let data = unsafe { idev.as_ref().drvdata_obtain::<T>() };
193+
let data = unsafe { idev.as_ref().drvdata_borrow::<T>() };
185194

186-
T::shutdown(idev, data.as_ref());
195+
T::shutdown(idev, data);
187196
}
188197

189198
/// The [`i2c::IdTable`] of the corresponding driver.

rust/kernel/io.rs

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,8 @@ macro_rules! define_read {
142142
/// Bound checks are performed on compile time, hence if the offset is not known at compile
143143
/// time, the build will fail.
144144
$(#[$attr])*
145-
#[inline]
145+
// Always inline to optimize out error path of `io_addr_assert`.
146+
#[inline(always)]
146147
pub fn $name(&self, offset: usize) -> $type_name {
147148
let addr = self.io_addr_assert::<$type_name>(offset);
148149

@@ -171,7 +172,8 @@ macro_rules! define_write {
171172
/// Bound checks are performed on compile time, hence if the offset is not known at compile
172173
/// time, the build will fail.
173174
$(#[$attr])*
174-
#[inline]
175+
// Always inline to optimize out error path of `io_addr_assert`.
176+
#[inline(always)]
175177
pub fn $name(&self, value: $type_name, offset: usize) {
176178
let addr = self.io_addr_assert::<$type_name>(offset);
177179

@@ -239,7 +241,8 @@ impl<const SIZE: usize> Io<SIZE> {
239241
self.addr().checked_add(offset).ok_or(EINVAL)
240242
}
241243

242-
#[inline]
244+
// Always inline to optimize out error path of `build_assert`.
245+
#[inline(always)]
243246
fn io_addr_assert<U>(&self, offset: usize) -> usize {
244247
build_assert!(Self::offset_valid::<U>(offset, SIZE));
245248

0 commit comments

Comments
 (0)