Re: [PATCH v5 09/11] drm/bridge: it6505: hold endpoint OF node reference while parsing it

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The local variable `link_frequencies` is used uninitialized if `of_property_read_variable_u64_array()` returns 0.
--

--- Patch [9]: [PATCH v5 09/11] drm/bridge: it6505: hold endpoint OF node reference while parsing it ---
commit a152f40f457f0c59599e70de71a80bcac957ec14
Author: Daniel Golle <daniel@makrotopia.org>
Subject: drm/bridge: it6505: hold endpoint OF node reference while parsing it

This commit fixes an issue where the device tree endpoint node reference
is dropped before parsing is complete, which can cause a use-after-free
race if the node is freed dynamically. It defers the of_node_put() calls
to execute only after all property parsing has finished.

> diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
> index e9208d827ce3d..6f23203d5f2a6 100644
> --- a/drivers/gpu/drm/bridge/ite-it6505.c
> +++ b/drivers/gpu/drm/bridge/ite-it6505.c
[ ... ]
>  	ep = of_graph_get_endpoint_by_regs(np, 0, 0);
> -	of_node_put(ep);
>  
>  	if (ep) {
>  		len = of_property_read_variable_u64_array(ep,
>  							  "link-frequencies",
>  							  &link_frequencies, 0,
>  							  1);
>  		if (len >= 0) {
>  			do_div(link_frequencies, 1000);

[Severity: High]
This isn't a bug introduced by this patch, but there is a pre-existing issue
here in it6505_parse_dt(). If of_property_read_variable_u64_array() returns
0, does this code use an uninitialized link_frequencies stack variable?

The of_property_read_variable_u64_array() function is called with a minimum
size of 0. If the link-frequencies property in the device tree is empty or
less than 8 bytes long (such as a common syntax mistake where a 32-bit
integer is provided), the function computes the size as 0. It then returns
0 without modifying the link_frequencies output variable.

Because the subsequent check is "if (len >= 0)", it evaluates to true, and
do_div() will consume whatever garbage value happens to be on the stack.
Depending on the stack memory, this could result in incorrectly rejecting
valid display modes or causing unpredictable display pipeline failures.

Should this code check "if (len > 0)" instead to guarantee the variable was
actually populated before using it?

>  			it6505->max_dpi_pixel_clock = link_frequencies;
>  		} else {
>  			dev_err(dev, "error link frequencies, use default");
>  			*max_dpi_pixel_clock = DPI_PIXEL_CLK_MAX;
>  		}
> +		of_node_put(ep);
>  	} else {
>  		dev_err(dev, "error endpoint, use default");
>  		*max_dpi_pixel_clock = DPI_PIXEL_CLK_MAX;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1784717129.git.daniel@makrotopia.org?part=9