Re: [PATCH v3 14/27] rust: i2c: make Driver trait lifetime-parameterized

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Use-After-Free due to I2C devres and subsystem teardown running before driver Data Drop.
- [High] Soundness hole allowing safe code to trigger UB via unchecked lifetime variance in GAT.
- [Low] Documentation mismatch in SAFETY comment for `drvdata_borrow`.
--

commit acb687d5f7c058a48caf341d55413d5a1b7e20ee
Author: Danilo Krummrich <dakr@kernel.org>

rust: i2c: make Driver trait lifetime-parameterized

This commit adds a 'bound lifetime parameter to the associated Data type
in the Driver trait. This allows driver implementations to capture the
device bound lifetime, enabling the storage of device resources by reference
rather than requiring Devres.

> diff --git a/rust/kernel/i2c.rs b/rust/kernel/i2c.rs
> index 5a4014f379d5b..7b92d42a2b98c 100644
> --- a/rust/kernel/i2c.rs
> +++ b/rust/kernel/i2c.rs
[ ... ]
> @@ -177,7 +177,7 @@ extern "C" fn remove_callback(idev: *mut bindings::i2c_client) {
>          // 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<KBox<T::Data>>`.

This isn't a bug, but the SAFETY comment still says Pin<KBox<T::Data>>
instead of T::Data<'_>.

> -        let data = unsafe { idev.as_ref().drvdata_borrow::<T::Data>() };
> +        let data = unsafe { idev.as_ref().drvdata_borrow::<T::Data<'_>>() };

Does retrieving the data via drvdata_borrow::<T::Data<'_>>() bypass
lifetime checking and introduce a soundness regression?

Because the Driver::Data associated type is not constrained to be
covariant, a driver could define an invariant Data type (for example,
containing a Cell<Option<&'bound I2cClient>>).

During unbind(), the driver could write a short-lived reference into the
Data struct. When remove_callback returns, this reference expires,
leaving a dangling pointer.

If that pointer is accessed in Drop, could it violate Rust's aliasing
rules and trigger undefined behavior?

[ ... ]
> @@ -189,7 +189,7 @@ extern "C" fn shutdown_callback(idev: *mut bindings::i2c_client) {
>          // 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<KBox<T::Data>>`.

This isn't a bug, but the SAFETY comment here also refers to
Pin<KBox<T::Data>> instead of reflecting the new 'bound lifetime.

> -        let data = unsafe { idev.as_ref().drvdata_borrow::<T::Data>() };
> +        let data = unsafe { idev.as_ref().drvdata_borrow::<T::Data<'_>>() };
[ ... ]
> @@ -320,7 +323,7 @@ pub trait Driver: Send {
>      type IdInfo: 'static;
>  
>      /// The type of the driver's bus device private data.
> -    type Data;
> +    type Data<'bound>: 'bound;

If a driver uses the 'bound lifetime to hold references to devres-managed
resources or I2C subsystem resources (like debugfs entries), will this lead
to a use-after-free regression?

In the I2C subsystem, i2c_device_remove() explicitly calls
devres_release_group() and cleans up subsystem state after the driver's
remove callback completes. The driver's Data is dropped later when the
driver core calls post_unbind_rust via device_unbind_cleanup().

By the time Drop is called on Data, wouldn't all devres resources already
be freed, causing a use-after-free if they are accessed during Drop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260517000149.3226762-1-dakr@kernel.org?part=14