Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Critical] `Panel::new` steals the devres-owned reference count initialized by `__devm_drm_panel_alloc`, leading to a double-free on device unbind or drop. - [Critical] The destructor of the driver data `T` is tied to device unbind via devres, rather than the panel's actual reference count, creating a split-lifetime Use-After-Free. - [High] Missing `Send`, `Sync`, and `'static` bounds on the driver data type `T` in `Panel::new` allows data races and Use-After-Free via non-thread-safe or short-lived types. Pre-existing issues: - [Critical] `Panel::from_of_node` improperly converts the result of `of_drm_find_panel` into an `ARef<Panel>` without incrementing the refcount, causing a premature free. -- --- Patch [4]: [PATCH 4/5] rust: drm: add panel producer abstractions --- commit 0e243089bb12e5256a62894a39811280cadcb027 Author: Albert Esteve <[email protected]> Subject: rust: drm: add panel producer abstractions This commit adds abstractions for DRM panel drivers, introducing the PanelFuncs trait and PanelContainer struct. It provides devres-managed allocation via __devm_drm_panel_alloc and ties the lifetime of the data to the device. > diff --git a/rust/kernel/drm/panel.rs b/rust/kernel/drm/panel.rs > index fd21cc2236685..8f87774e06ed7 100644 > --- a/rust/kernel/drm/panel.rs > +++ b/rust/kernel/drm/panel.rs [ ... ] > @@ -139,6 +143,66 @@ pub fn from_of_node(node: &of::Node) -> > Result<ARef<Self>> { > // `of_drm_find_panel` returns a kref-incremented reference. > Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(panel).cast()) }) > } [Severity: Critical] This is a pre-existing issue, but does of_drm_find_panel actually increment the reference count? Looking at the C implementation, of_drm_find_panel iterates over the global panel_list and returns the pointer without calling kref_get or drm_panel_get. If we consume the pointer directly with ARef::from_raw(), dropping the ARef will decrement the refcount. Could this drop the refcount to zero and prematurely free the panel while the provider driver's devres still owns it? > + > + /// Allocates and initialises a device-managed panel. > + /// > + /// `data` is embedded in the same allocation as the `drm_panel` and its > + /// destructor is called automatically when `dev` is unbound. > + /// > + /// Use [`Registration::register`] to add the panel to the global > registry > + /// once it is ready to be used by display drivers. > + pub fn new<T: PanelFuncs>( > + dev: &Device, > + data: T, > + connector_type: ConnectorType, > + ) -> Result<ARef<Self>> { [Severity: High] Does T need Send, Sync, and 'static bounds here? Since Panel::new takes ownership of T and stores it in a heap allocation accessed by the concurrent C DRM subsystem, safe Rust code could provide a non-thread-safe type (like one containing Cell or Rc) or a short-lived reference. Would this allow data races or use-after-free when the C DRM core concurrently invokes vtable callbacks? > + // SAFETY: `dev` is valid by its type invariants; > `PanelFuncsVTable::build()` > + // returns a valid, static `drm_panel_funcs` pointer. > + let container = error::from_err_ptr(unsafe { > + bindings::__devm_drm_panel_alloc( > + dev.as_raw(), > + core::mem::size_of::<PanelContainer<T>>(), > + core::mem::offset_of!(PanelContainer<T>, panel), > + PanelFuncsVTable::<T>::build(), > + connector_type as i32, > + ) > + })? as *mut PanelContainer<T>; > + > + // SAFETY: `container` is a valid pointer to uninitialized memory. > + unsafe { > + core::ptr::write( > + core::ptr::addr_of_mut!((*container).data), > + ManuallyDrop::new(data), > + ) > + }; > + > + // SAFETY: > + // - `dev.as_raw()` is a pointer to a valid and bound device. > + // - `container.cast()` is a valid pointer to the initialized > `PanelContainer<T>`. > + error::to_result(unsafe { > + // `devm_add_action_or_reset` calls `drop_panel_data` on > failure, so `data` > + // is dropped even if this registration fails. > + // Registering after `__devm_drm_panel_alloc` ensures devres > LIFO order: > + // `drop_panel_data` runs before `kfree(container)`. > + bindings::devm_add_action_or_reset( > + dev.as_raw(), > + Some(drop_panel_data::<T>), > + container.cast(), > + ) [Severity: Critical] Is it safe to tie the destruction of T to device unbind rather than the panel's reference count? Since the panel is reference-counted and exposed as an ARef<Panel>, consumer drivers could keep the panel alive after the provider device unbinds. When unbind occurs, drop_panel_data will destroy T. If a consumer later calls a panel method like get_modes, the C code will invoke the Rust vtable callbacks which operate on the dropped T, potentially leading to a use-after-free. > + })?; > + > + // SAFETY: `__devm_drm_panel_alloc` was successful, hence > `container` is > + // valid and the `drm_panel` at this offset is initialised. > + let raw = unsafe { > + (container as *mut u8) > + .add(core::mem::offset_of!(PanelContainer<T>, panel)) > + .cast::<bindings::drm_panel>() > + }; > + > + // SAFETY: `__devm_drm_panel_alloc` was successful, hence `raw` is > valid > + // and the refcount is non-zero. > + Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(raw).cast()) }) [Severity: Critical] Does ARef::from_raw steal the devres-owned reference count here? __devm_drm_panel_alloc initializes the panel's refcount to 1 and immediately registers a devres action (drm_panel_put_void) that owns this reference. Wrapping the raw pointer directly with ARef::from_raw without incrementing the refcount means both ARef and devres think they own the same refcount. When the ARef is dropped, won't it free the panel via __drm_panel_free, causing a double-free later when device unbind triggers the devres action? > + } > } [ ... ] > +/// Operations implemented by a DRM panel driver. > +/// > +/// Implement this trait to provide a DRM panel driver and its callbacks. Use > +/// [`Panel::new`] to allocate the panel, passing the driver data as `T`. > +/// > +/// C header: [`include/drm/drm_panel.h`](srctree/include/drm/drm_panel.h) > +#[vtable] > +pub trait PanelFuncs { [Severity: High] Should this trait require Send, Sync, and 'static supertraits? Similar to the missing bounds in Panel::new(), without these supertraits, types implementing PanelFuncs might contain thread-unsafe primitives or short-lived references, which could cause data races or use-after-free when accessed concurrently by the DRM subsystem. > + /// Turn on panel and perform set up. [ ... ] > +// Devres action: run T's destructor before `kfree(container)`. > +// > +// # Safety > +// > +// `ptr` must be the base of a live `PanelContainer<T>` whose `data` field > was > +// initialised by `Panel::new` and has not yet been dropped. > +unsafe extern "C" fn drop_panel_data<T>(ptr: *mut core::ffi::c_void) { > + // SAFETY: Caller guarantees `ptr` is the base of a live > `PanelContainer<T>` > + // with an initialised `data` field. `data` is at offset 0, so `ptr as > *mut T` > + // is valid. > + unsafe { core::ptr::drop_in_place(ptr as *mut T) }; [Severity: Critical] Does this execute the destructor while the container might still be alive? If a consumer driver still holds an ARef<Panel>, the container's reference count is non-zero. Since drop_panel_data is called unconditionally on device unbind, won't drop_in_place destroy T while it could still be accessed via the panel's vtable callbacks? > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
