From 4181aceb4af414bd6d2ce5eb9a22637bbb4f5f8c Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 7 Jan 2026 11:35:00 +0100 Subject: rust: i2c: do not drop device private data on shutdown() We must not drop the device private data on shutdown(); none of the registrations attached to devres that might access the device private data are released before shutdown() is called. Hence, freeing the device private data on shutdown() can cause UAF bugs. Fixes: 57c5bd9aee94 ("rust: i2c: add basic I2C device and driver abstractions") Acked-by: Alice Ryhl Acked-by: Igor Korotin Link: https://patch.msgid.link/20260107103511.570525-2-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/i2c.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'rust/kernel/i2c.rs') diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs index 491e6cc25cf4..35b678b78d91 100644 --- a/rust/kernel/i2c.rs +++ b/rust/kernel/i2c.rs @@ -181,9 +181,9 @@ impl Adapter { // SAFETY: `shutdown_callback` is only ever called after a successful call to // `probe_callback`, hence it's guaranteed that `Device::set_drvdata()` has been called // and stored a `Pin>`. - let data = unsafe { idev.as_ref().drvdata_obtain::() }; + let data = unsafe { idev.as_ref().drvdata_borrow::() }; - T::shutdown(idev, data.as_ref()); + T::shutdown(idev, data); } /// The [`i2c::IdTable`] of the corresponding driver. -- cgit v1.2.3 From 0af1a9e4629a85964a7eebe58ebd2ca37c8c21fc Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 7 Jan 2026 11:35:02 +0100 Subject: rust: driver: introduce a DriverLayout trait The DriverLayout trait describes the layout of a specific driver structure, such as `struct pci_driver` or `struct platform_driver`. In a first step, this replaces the associated type RegType of the RegistrationOps with the DriverLayout::DriverType associated type. Acked-by: Alice Ryhl Acked-by: Igor Korotin Link: https://patch.msgid.link/20260107103511.570525-4-dakr@kernel.org [ Rename driver::Driver to driver::DriverLayout, as it represents the layout of a driver structure rather than the driver structure itself. - Danilo ] Signed-off-by: Danilo Krummrich --- rust/kernel/i2c.rs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) (limited to 'rust/kernel/i2c.rs') diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs index 35b678b78d91..8e80d8572e1a 100644 --- a/rust/kernel/i2c.rs +++ b/rust/kernel/i2c.rs @@ -92,13 +92,17 @@ macro_rules! i2c_device_table { /// An adapter for the registration of I2C drivers. pub struct Adapter(T); -// SAFETY: A call to `unregister` for a given instance of `RegType` is guaranteed to be valid if +// SAFETY: +// - `bindings::i2c_driver` is a C type declared as `repr(C)`. +unsafe impl driver::DriverLayout for Adapter { + type DriverType = bindings::i2c_driver; +} + +// SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if // a preceding call to `register` has been successful. unsafe impl driver::RegistrationOps for Adapter { - type RegType = bindings::i2c_driver; - unsafe fn register( - idrv: &Opaque, + idrv: &Opaque, name: &'static CStr, module: &'static ThisModule, ) -> Result { @@ -133,12 +137,12 @@ unsafe impl driver::RegistrationOps for Adapter { (*idrv.get()).driver.acpi_match_table = acpi_table; } - // SAFETY: `idrv` is guaranteed to be a valid `RegType`. + // SAFETY: `idrv` is guaranteed to be a valid `DriverType`. to_result(unsafe { bindings::i2c_register_driver(module.0, idrv.get()) }) } - unsafe fn unregister(idrv: &Opaque) { - // SAFETY: `idrv` is guaranteed to be a valid `RegType`. + unsafe fn unregister(idrv: &Opaque) { + // SAFETY: `idrv` is guaranteed to be a valid `DriverType`. unsafe { bindings::i2c_del_driver(idrv.get()) } } } -- cgit v1.2.3 From c1d4519e1c36ffa01973e23af4502e69dcd84f39 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 7 Jan 2026 11:35:03 +0100 Subject: rust: driver: add DEVICE_DRIVER_OFFSET to the DriverLayout trait Add an associated const DEVICE_DRIVER_OFFSET to the DriverLayout trait indicating the offset of the embedded struct device_driver within Self::DriverType, i.e. the specific driver structs, such as struct pci_driver or struct platform_driver. Acked-by: Alice Ryhl Acked-by: Igor Korotin Link: https://patch.msgid.link/20260107103511.570525-5-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/i2c.rs | 3 +++ 1 file changed, 3 insertions(+) (limited to 'rust/kernel/i2c.rs') diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs index 8e80d8572e1a..d97e73282003 100644 --- a/rust/kernel/i2c.rs +++ b/rust/kernel/i2c.rs @@ -94,8 +94,11 @@ pub struct Adapter(T); // SAFETY: // - `bindings::i2c_driver` is a C type declared as `repr(C)`. +// - `struct i2c_driver` embeds a `struct device_driver`. +// - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`. unsafe impl driver::DriverLayout for Adapter { type DriverType = bindings::i2c_driver; + const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver); } // SAFETY: A call to `unregister` for a given instance of `DriverType` is guaranteed to be valid if -- cgit v1.2.3 From 2ad0f490c224283eb5b38f81e247000ce3c714d3 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 7 Jan 2026 11:35:04 +0100 Subject: rust: driver: add DriverData type to the DriverLayout trait Add an associated type DriverData to the DriverLayout trait indicating the type of the driver's device private data. Acked-by: Alice Ryhl Acked-by: Igor Korotin Link: https://patch.msgid.link/20260107103511.570525-6-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/i2c.rs | 2 ++ 1 file changed, 2 insertions(+) (limited to 'rust/kernel/i2c.rs') diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs index d97e73282003..e86242227081 100644 --- a/rust/kernel/i2c.rs +++ b/rust/kernel/i2c.rs @@ -94,10 +94,12 @@ pub struct Adapter(T); // SAFETY: // - `bindings::i2c_driver` is a C type declared as `repr(C)`. +// - `T` is the type of the driver's device private data. // - `struct i2c_driver` embeds a `struct device_driver`. // - `DEVICE_DRIVER_OFFSET` is the correct byte offset to the embedded `struct device_driver`. unsafe impl driver::DriverLayout for Adapter { type DriverType = bindings::i2c_driver; + type DriverData = T; const DEVICE_DRIVER_OFFSET: usize = core::mem::offset_of!(Self::DriverType, driver); } -- cgit v1.2.3 From a995fe1a3aa78b7d06cc1cc7b6b8436c5e93b07f Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 7 Jan 2026 11:35:05 +0100 Subject: rust: driver: drop device private data post unbind Currently, the driver's device private data is allocated and initialized from driver core code called from bus abstractions after the driver's probe() callback returned the corresponding initializer. Similarly, the driver's device private data is dropped within the remove() callback of bus abstractions after calling the remove() callback of the corresponding driver. However, commit 6f61a2637abe ("rust: device: introduce Device::drvdata()") introduced an accessor for the driver's device private data for a Device, i.e. a device that is currently bound to a driver. Obviously, this is in conflict with dropping the driver's device private data in remove(), since a device can not be considered to be fully unbound after remove() has finished: We also have to consider registrations guarded by devres - such as IRQ or class device registrations - which are torn down after remove() in devres_release_all(). Thus, it can happen that, for instance, a class device or IRQ callback still calls Device::drvdata(), which then runs concurrently to remove() (which sets dev->driver_data to NULL and drops the driver's device private data), before devres_release_all() started to tear down the corresponding registration. This is because devres guarded registrations can, as expected, access the corresponding Device that defines their scope. In C it simply is the driver's responsibility to ensure that its device private data is freed after e.g. an IRQ registration is unregistered. Typically, C drivers achieve this by allocating their device private data with e.g. devm_kzalloc() before doing anything else, i.e. before e.g. registering an IRQ with devm_request_threaded_irq(), relying on the reverse order cleanup of devres. Technically, we could do something similar in Rust. However, the resulting code would be pretty messy: In Rust we have to differentiate between allocated but uninitialized memory and initialized memory in the type system. Thus, we would need to somehow keep track of whether the driver's device private data object has been initialized (i.e. probe() was successful and returned a valid initializer for this memory) and conditionally call the destructor of the corresponding object when it is freed. This is because we'd need to allocate and register the memory of the driver's device private data *before* it is initialized by the initializer returned by the driver's probe() callback, because the driver could already register devres guarded registrations within probe() outside of the driver's device private data initializer. Luckily there is a much simpler solution: Instead of dropping the driver's device private data at the end of remove(), we just drop it after the device has been fully unbound, i.e. after all devres callbacks have been processed. For this, we introduce a new post_unbind() callback private to the driver-core, i.e. the callback is neither exposed to drivers, nor to bus abstractions. This way, the driver-core code can simply continue to conditionally allocate the memory for the driver's device private data when the driver's initializer is returned from probe() - no change needed - and drop it when the driver-core code receives the post_unbind() callback. Closes: https://lore.kernel.org/all/DEZMS6Y4A7XE.XE7EUBT5SJFJ@kernel.org/ Fixes: 6f61a2637abe ("rust: device: introduce Device::drvdata()") Acked-by: Alice Ryhl Acked-by: Greg Kroah-Hartman Acked-by: Igor Korotin Link: https://patch.msgid.link/20260107103511.570525-7-dakr@kernel.org [ Remove #ifdef CONFIG_RUST, rename post_unbind() to post_unbind_rust(). - Danilo] Signed-off-by: Danilo Krummrich --- rust/kernel/i2c.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'rust/kernel/i2c.rs') diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs index e86242227081..39b0a9a207fd 100644 --- a/rust/kernel/i2c.rs +++ b/rust/kernel/i2c.rs @@ -178,9 +178,9 @@ impl Adapter { // SAFETY: `remove_callback` is only ever called after a successful call to // `probe_callback`, hence it's guaranteed that `I2cClient::set_drvdata()` has been called // and stored a `Pin>`. - let data = unsafe { idev.as_ref().drvdata_obtain::() }; + let data = unsafe { idev.as_ref().drvdata_borrow::() }; - T::unbind(idev, data.as_ref()); + T::unbind(idev, data); } extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) { -- cgit v1.2.3