diff mbox series

[v2,4/5] drm/panel: find_panel_by_fwnode() return a counted reference

Message ID 20260713-drm_refcount_wiring-v2-4-d3bb61f4bd4d@redhat.com (mailing list archive)
State New
Headers
Series drm/panel: refcounting panel lookups and references |

Commit Message

Albert Esteve July 13, 2026, 11:53 a.m. UTC 
find_panel_by_fwnode() is the fwnode-based counterpart to
of_drm_find_panel(), used internally by drm_panel_add_follower().
Like of_drm_find_panel(), it returned an unrefcounted pointer,
leaving a window where the panel could be freed between the lookup
and first use.

drm_panel_add_follower() worked around the missing panel kref by
calling get_device() on the panel's underlying struct device.
However, get_device() only prevents the device kobject from being freed.
It does not prevent the panel's kzalloc()'d container memory from being
released when the kref reaches zero.

Apply the same fix: call drm_panel_get() under panel_lock before
returning. Since find_panel_by_fwnode() now transfers a counted
reference to drm_panel_add_follower(), drm_panel_remove_follower()
must balance it with a matching drm_panel_put().

Signed-off-by: Albert Esteve <aesteve@redhat.com>
---
 drivers/gpu/drm/drm_panel.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Comments

sashiko-bot@kernel.org July 13, 2026, 12:08 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Drivers calling of_drm_find_panel() directly leak the returned panel reference.

Pre-existing issues:
- [High] TOCTOU race in drm_panel_add_follower() when acquiring the panel's underlying device, leading to a potential Use-After-Free of panel->dev.
--

--- Patch [4]: [PATCH v2 4/5] drm/panel: find_panel_by_fwnode() return a counted reference ---
commit 5cd4c479944cf29f6f5aa4a3e31207bf48dc0567
Author: Albert Esteve <aesteve@redhat.com>

drm/panel: find_panel_by_fwnode() return a counted reference

This commit updates find_panel_by_fwnode() to return a refcounted pointer
to the panel, mirroring a similar fix made to of_drm_find_panel(). It calls
drm_panel_get() under the panel_lock before returning, ensuring the memory
is not freed prematurely. Callers are now responsible for balancing this
with drm_panel_put(), which is added to drm_panel_remove_follower().

> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index 72cf86253c0cd..a00ae98ed0956 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -542,7 +542,13 @@ int of_drm_get_panel_orientation(const struct device_node *np,
>  EXPORT_SYMBOL(of_drm_get_panel_orientation);
>  #endif
>  
> -/* Find panel by fwnode. This should be identical to of_drm_find_panel(). */
> +/*
> + * Find panel by fwnode, returning a counted reference.
> + *
> + * Behaves identically to of_drm_find_panel(). On success the returned
> + * pointer has been passed through drm_panel_get(); the caller must call
> + * drm_panel_put() when done with it.
> + */

[Severity: Medium]
Since the earlier patch in this series updated of_drm_find_panel() to
also return a counted reference, do drivers that call it directly need
to drop their reference to prevent a memory leak?

For example, in bridge drivers like lvds-codec.c:

drivers/gpu/drm/bridge/lvds-codec.c:lvds_codec_probe() {
    panel = of_drm_find_panel(panel_node);
    of_node_put(panel_node);
    ...
    lvds_codec->panel_bridge = devm_drm_panel_bridge_add_typed(dev, panel,
                                                lvds_codec->connector_type);
}

Because devm_drm_panel_bridge_add_typed() takes its own reference to the
panel via drm_panel_get(), does this code leak the initial reference
returned by of_drm_find_panel() since drm_panel_put() is never called?

This same leak pattern appears to exist in other callers as well, such as
samsung-dsim.c:samsung_dsim_host_attach() and
rcar_du_encoder.c:rcar_du_encoder_init().

>  static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle *fwnode)
>  {
>  	struct drm_panel *panel;
> @@ -554,6 +560,7 @@ static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle *fwnode
>  
>  	list_for_each_entry(panel, &panel_list, list) {
>  		if (dev_fwnode(panel->dev) == fwnode) {
> +			drm_panel_get(panel);
>  			mutex_unlock(&panel_lock);

[Severity: High]
This is a pre-existing issue, but is there a time-of-check to time-of-use
race condition when callers attempt to pin the underlying device?

When find_panel_by_fwnode() (or find_panel_by_dev()) returns, the panel's
internal refcount is incremented under panel_lock, but the underlying
struct device (panel->dev) is not pinned.

If a concurrent unbind occurs before the caller pins the device, the device
memory could be freed. For example, looking at drm_panel_add_follower():

drivers/gpu/drm/drm_panel.c:drm_panel_add_follower() {
    panel = find_panel_by_dev(follower_dev);
    if (IS_ERR(panel))
        return PTR_ERR(panel);

    get_device(panel->dev);
}

If the panel device driver unbinds immediately after panel_lock is dropped,
could get_device(panel->dev) trigger a use-after-free on the destroyed
device?

>  			return panel;
>  		}

[ ... ]
Albert Esteve July 13, 2026, 12:50 p.m. UTC | #2 
On Mon, Jul 13, 2026 at 2:08 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> New issues:
> - [Medium] Drivers calling of_drm_find_panel() directly leak the returned panel reference.
>
> Pre-existing issues:
> - [High] TOCTOU race in drm_panel_add_follower() when acquiring the panel's underlying device, leading to a potential Use-After-Free of panel->dev.
> --
>
> --- Patch [4]: [PATCH v2 4/5] drm/panel: find_panel_by_fwnode() return a counted reference ---
> commit 5cd4c479944cf29f6f5aa4a3e31207bf48dc0567
> Author: Albert Esteve <aesteve@redhat.com>
>
> drm/panel: find_panel_by_fwnode() return a counted reference
>
> This commit updates find_panel_by_fwnode() to return a refcounted pointer
> to the panel, mirroring a similar fix made to of_drm_find_panel(). It calls
> drm_panel_get() under the panel_lock before returning, ensuring the memory
> is not freed prematurely. Callers are now responsible for balancing this
> with drm_panel_put(), which is added to drm_panel_remove_follower().
>
> > diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> > index 72cf86253c0cd..a00ae98ed0956 100644
> > --- a/drivers/gpu/drm/drm_panel.c
> > +++ b/drivers/gpu/drm/drm_panel.c
> > @@ -542,7 +542,13 @@ int of_drm_get_panel_orientation(const struct device_node *np,
> >  EXPORT_SYMBOL(of_drm_get_panel_orientation);
> >  #endif
> >
> > -/* Find panel by fwnode. This should be identical to of_drm_find_panel(). */
> > +/*
> > + * Find panel by fwnode, returning a counted reference.
> > + *
> > + * Behaves identically to of_drm_find_panel(). On success the returned
> > + * pointer has been passed through drm_panel_get(); the caller must call
> > + * drm_panel_put() when done with it.
> > + */
>
> [Severity: Medium]
> Since the earlier patch in this series updated of_drm_find_panel() to
> also return a counted reference, do drivers that call it directly need
> to drop their reference to prevent a memory leak?
>
> For example, in bridge drivers like lvds-codec.c:
>
> drivers/gpu/drm/bridge/lvds-codec.c:lvds_codec_probe() {
>     panel = of_drm_find_panel(panel_node);
>     of_node_put(panel_node);
>     ...
>     lvds_codec->panel_bridge = devm_drm_panel_bridge_add_typed(dev, panel,
>                                                 lvds_codec->connector_type);
> }
>
> Because devm_drm_panel_bridge_add_typed() takes its own reference to the
> panel via drm_panel_get(), does this code leak the initial reference
> returned by of_drm_find_panel() since drm_panel_put() is never called?
>
> This same leak pattern appears to exist in other callers as well, such as
> samsung-dsim.c:samsung_dsim_host_attach() and
> rcar_du_encoder.c:rcar_du_encoder_init().

Uh this is a real issue that I missed in several callers. I will add
drm_panel_put() after the bridge creation where needed in the next
version.

BR,
Albert.

>
> >  static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle *fwnode)
> >  {
> >       struct drm_panel *panel;
> > @@ -554,6 +560,7 @@ static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle *fwnode
> >
> >       list_for_each_entry(panel, &panel_list, list) {
> >               if (dev_fwnode(panel->dev) == fwnode) {
> > +                     drm_panel_get(panel);
> >                       mutex_unlock(&panel_lock);
>
> [Severity: High]
> This is a pre-existing issue, but is there a time-of-check to time-of-use
> race condition when callers attempt to pin the underlying device?
>
> When find_panel_by_fwnode() (or find_panel_by_dev()) returns, the panel's
> internal refcount is incremented under panel_lock, but the underlying
> struct device (panel->dev) is not pinned.
>
> If a concurrent unbind occurs before the caller pins the device, the device
> memory could be freed. For example, looking at drm_panel_add_follower():
>
> drivers/gpu/drm/drm_panel.c:drm_panel_add_follower() {
>     panel = find_panel_by_dev(follower_dev);
>     if (IS_ERR(panel))
>         return PTR_ERR(panel);
>
>     get_device(panel->dev);
> }
>
> If the panel device driver unbinds immediately after panel_lock is dropped,
> could get_device(panel->dev) trigger a use-after-free on the destroyed
> device?
>
> >                       return panel;
> >               }
>
> [ ... ]
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260713-drm_refcount_wiring-v2-0-d3bb61f4bd4d@redhat.com?part=4
>
Maxime Ripard July 17, 2026, 8:54 a.m. UTC | #3 
On Mon, 13 Jul 2026 13:53:07 +0200, Albert Esteve wrote:
> find_panel_by_fwnode() is the fwnode-based counterpart to
> of_drm_find_panel(), used internally by drm_panel_add_follower().
> Like of_drm_find_panel(), it returned an unrefcounted pointer,
> leaving a window where the panel could be freed between the lookup
> and first use.
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index 72cf86253c0cd..a00ae98ed0956 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -542,7 +542,13 @@  int of_drm_get_panel_orientation(const struct device_node *np,
 EXPORT_SYMBOL(of_drm_get_panel_orientation);
 #endif
 
-/* Find panel by fwnode. This should be identical to of_drm_find_panel(). */
+/*
+ * Find panel by fwnode, returning a counted reference.
+ *
+ * Behaves identically to of_drm_find_panel(). On success the returned
+ * pointer has been passed through drm_panel_get(); the caller must call
+ * drm_panel_put() when done with it.
+ */
 static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle *fwnode)
 {
 	struct drm_panel *panel;
@@ -554,6 +560,7 @@  static struct drm_panel *find_panel_by_fwnode(const struct fwnode_handle *fwnode
 
 	list_for_each_entry(panel, &panel_list, list) {
 		if (dev_fwnode(panel->dev) == fwnode) {
+			drm_panel_get(panel);
 			mutex_unlock(&panel_lock);
 			return panel;
 		}
@@ -690,6 +697,7 @@  void drm_panel_remove_follower(struct drm_panel_follower *follower)
 	mutex_unlock(&panel->follower_lock);
 
 	put_device(panel->dev);
+	drm_panel_put(panel);
 }
 EXPORT_SYMBOL(drm_panel_remove_follower);