Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] Refcount underflow and use-after-free vulnerability in `Panel::from_of_node`. - [High] Soundness hole in `Registration::register` allowing memory corruption from safe Rust. - [Low] Missing conditional compilation guard for `panel` module causing build failures. - [Low] Improper `#[cfg]` guard on `of_backlight` breaks the build for module configurations. - [Low] Missing `#[inline]` annotations on small forwarding functions. - [Low] Imports violate the kernel vertical import style. --
--- Patch [3]: [PATCH 3/5] rust: drm: add panel consumer abstractions --- commit e330c1b98aba78a95f008b2fccb2105d337a8df8 Author: Albert Esteve <[email protected]> rust: drm: add panel consumer abstractions This commit adds a Rust abstraction for the DRM panel subsystem, providing wrappers for the consumer side of the panel API. It introduces the `Panel`, `PanelOrientation`, and `Registration` types, handling refcounting and device tree lookups. > diff --git a/rust/kernel/drm/mod.rs b/rust/kernel/drm/mod.rs > index 672ea8728e1c3..d1de5d448a1a5 100644 > --- a/rust/kernel/drm/mod.rs > +++ b/rust/kernel/drm/mod.rs > @@ -9,6 +9,7 @@ > pub mod gem; > pub mod gpuvm; > pub mod ioctl; > +pub mod panel; [Severity: Low] Does this module need a conditional compilation guard? If `CONFIG_DRM=y` but `CONFIG_DRM_PANEL` is disabled, `panel.rs` will still be compiled. However, `panel.rs` relies on C functions like `drm_panel_prepare` which become static inline dummies in C when the config is disabled, meaning bindgen won't export them and the Rust build will fail. [ ... ] > diff --git a/rust/kernel/drm/panel.rs b/rust/kernel/drm/panel.rs > new file mode 100644 > index 0000000000000..fd21cc2236685 > --- /dev/null > +++ b/rust/kernel/drm/panel.rs > @@ -0,0 +1,227 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! DRM panel abstractions. > +//! > +//! C header: [`include/drm/drm_panel.h`](srctree/include/drm/drm_panel.h) > + > +use crate::drm::connector::Connector; > +use crate::{ > + bindings, error, of, > + prelude::*, > + sync::aref::{ARef, AlwaysRefCounted}, > + types::Opaque, > +}; [Severity: Low] Could these imports be reformatted to follow the kernel's vertical import style guidelines? [ ... ] > + /// Returns the raw pointer to the underlying `struct drm_panel`. > + pub fn as_raw(&self) -> *mut bindings::drm_panel { > + self.0.get() > + } [Severity: Low] Should small forwarding functions like this one and `prepare()` below have `#[inline]` annotations, as recommended by the Rust subsystem guidelines for abstractions? [ ... ] > + /// A typical implementation for a panel driver supporting device tree > + /// will call this function at probe time. Backlight will then be handled > + /// transparently without requiring any intervention from the driver. > + #[cfg(CONFIG_BACKLIGHT_CLASS_DEVICE)] > + pub fn of_backlight(&self) -> Result<()> { > + // SAFETY: The type invariant guarantees the pointer is valid. > + error::to_result(unsafe { > bindings::drm_panel_of_backlight(self.as_raw()) })?; > + Ok(()) > + } [Severity: Low] Does this `#[cfg]` attribute handle the module build correctly? If `CONFIG_BACKLIGHT_CLASS_DEVICE=m`, the `#[cfg(CONFIG_BACKLIGHT_CLASS_DEVICE)]` attribute evaluates to false, which entirely strips the `of_backlight` method from the Rust API. This would cause build failures for any Rust driver trying to call it, even though the C fallback `drm_panel_of_backlight` stub exists. > + > + /// Look up the panel associated with the given device tree node. > + /// > + /// Searches the set of registered panels for one that matches the given > device > + /// tree node. If a matching panel is found, return a pointer to it. > + pub fn from_of_node(node: &of::Node) -> Result<ARef<Self>> { > + // SAFETY: `node.as_raw()` is a valid device_node pointer. > + let panel = error::from_err_ptr(unsafe { > bindings::of_drm_find_panel(node.as_raw()) })?; > + > + // SAFETY: `from_err_ptr` guarantees a non-null pointer on success. > + // `of_drm_find_panel` returns a kref-incremented reference. > + Ok(unsafe { ARef::from_raw(NonNull::new_unchecked(panel).cast()) }) > + } [Severity: High] Does `of_drm_find_panel` actually increment the refcount as the comment states? Looking at the C implementation, `of_drm_find_panel()` returns a raw pointer but does not increment the panel's `kref` internally. By wrapping this pointer directly in `ARef::from_raw()`, the Rust code assumes ownership of a refcount increment that didn't happen. When the resulting `ARef` is dropped, it will call `drm_panel_put()` and cause a refcount underflow, leading to a use-after-free. [ ... ] > +/// A registration of a panel to the global panel registry. > +pub struct Registration(ARef<Panel>); > + > +impl Registration { > + /// Registers a panel with the global panel registry. > + pub fn register(panel: ARef<Panel>) -> Self { > + // SAFETY: The type invariant guarantees the pointer is valid. > + unsafe { bindings::drm_panel_add(panel.as_raw()) }; > + Self(panel) > + } [Severity: High] Is it safe to expose this as a safe function taking an `ARef<Panel>`? If a safe caller passes an already registered panel (for instance, one obtained via `from_of_node`, or by cloning the `ARef` and calling `Registration::register()` twice), this will unconditionally call `drm_panel_add()`. In C, `drm_panel_add()` executes `list_add_tail(&panel->list, &panel_list)`. If the panel is already on a list, this will corrupt the global linked list pointers. This seems to allow memory corruption from entirely safe Rust code. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=3
