Re: [PATCH 3/5] rust: drm: add panel consumer abstractions

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/20260817-drm_panel_bindings-v1-0-1f974508a31c-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org?part=3