Re: [PATCH v10 06/11] drm/rockchip: analogix_dp: Fix OF node reference leak via auto cleanup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Stack buffer overflow caused by formatting an unbounded device tree node name into a fixed-size array, with the resulting string being completely unused.
- [High] Uninitialized variable `port_id` is read if `of_property_read_u32()` fails.
- [High] `drm_of_encoder_active_endpoint` returns an endpoint structure containing a dangling device_node pointer, causing a Use-After-Free.
- [High] The function calls `clk_prepare_enable()` and `clk_disable_unprepare()`, which can sleep, from within an atomic context.
--

commit 64019e228c078284dbae98e48cc02de08453152c
Author: Damon Ding <damon.ding@rock-chips.com>

drm/rockchip: analogix_dp: Fix OF node reference leak via auto cleanup

This patch converts the device node reference management in
rockchip_dp_drm_encoder_enable() to use the __free(device_node)
auto-cleanup helper. It aims to fix a reference leak where
of_node_put() was missed in early return paths.

Link: https://sashiko.dev/#/patchset/20260527024336.191433-1-damon.ding@rock-chips.com?part=5 #1

> diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
> @@ -206,7 +207,6 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,

[Severity: High]
This is a pre-existing issue, but does this function call sleeping functions
from an atomic context?

As the atomic_enable callback for the DRM encoder, this executes in an atomic
context where sleeping is forbidden. However, rockchip_dp_drm_encoder_enable()
calls clk_prepare_enable() and clk_disable_unprepare(). Both clock APIs
acquire a mutex internally and can sleep, which could cause kernel warnings
or deadlocks.

>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
>  	struct of_endpoint endpoint;

[ ... ]

> @@ -230,18 +230,22 @@ static void rockchip_dp_drm_encoder_enable(struct drm_encoder *encoder,
>  	if (ret < 0)
>  		return;
>  
> -	remote_port_parent = of_graph_get_remote_port_parent(endpoint.local_node);
> +	struct device_node *remote_port_parent __free(device_node) =
> +		of_graph_get_remote_port_parent(endpoint.local_node);

[Severity: High]
This isn't a bug introduced by this patch, but does the earlier call to
drm_of_encoder_active_endpoint() leave endpoint.local_node as a dangling
pointer?

Looking at drm_of_encoder_active_endpoint(), it parses the endpoint and
drops the reference before returning:

    ret = of_graph_parse_endpoint(ep, endpoint);
    of_node_put(ep);
    return ret;

When we pass endpoint.local_node to of_graph_get_remote_port_parent() here,
could this result in a use-after-free since the node's reference count was
already decremented?

>  	if (remote_port_parent) {
> -		if (of_get_child_by_name(remote_port_parent, "ports")) {
> -			remote_port = of_graph_get_remote_port(endpoint.local_node);
> +		struct device_node *ports __free(device_node) =
> +			of_get_child_by_name(remote_port_parent, "ports");
> +
> +		if (ports) {
> +			struct device_node *remote_port __free(device_node) =
> +				of_graph_get_remote_port(endpoint.local_node);
> +
>  			of_property_read_u32(remote_port, "reg", &port_id);

[Severity: High]
This is a pre-existing issue, but can port_id be left uninitialized if
of_property_read_u32() fails?

The port_id variable is declared on the stack without initialization. If the
"reg" property is missing or remote_port is NULL, port_id is never updated.

> -			of_node_put(remote_port);
>  			sprintf(name, "%s vp%d", remote_port_parent->full_name, port_id);

[Severity: High]
This isn't a bug introduced by this patch, but does this sprintf() write
potentially unbounded string data into the 32-byte name array?

Device tree node names (like remote_port_parent->full_name) can exceed 32
characters, which would cause a stack buffer overflow. Additionally, if
port_id was left uninitialized in the previous call, random integers could
be appended here.

Furthermore, is the populated name buffer ever used? The subsequent debug
print ignores it entirely:

    DRM_DEV_DEBUG(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");

Can this sprintf() and the name array be removed entirely?

>  		} else {
>  			sprintf(name, "%s %s",
>  				remote_port_parent->full_name, endpoint.id ? "vopl" : "vopb");
>  		}
> -		of_node_put(remote_port_parent);
>  
>  		DRM_DEV_DEBUG(dp->dev, "vop %s output to dp\n", (ret) ? "LIT" : "BIG");
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601065100.1103873-1-damon.ding@rock-chips.com?part=6