Re: [PATCH v2 2/6] video: logo: allow the boot logo to come from the device tree

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Premature freeing of device tree boot logo memory creates a race condition with asynchronous framebuffer probes, leading to a NULL pointer dereference.
- [Medium] Unbounded kmalloc() allocation for device tree boot logo triggers a kernel warning and fails for sizes exceeding KMALLOC_MAX_SIZE.
- [Medium] Silent fallback to searching the device tree root node if the /chosen node does not exist.
--

--- Patch [2]: [PATCH v2 2/6] video: logo: allow the boot logo to come from the device tree ---
commit f460a2fda249375abd332c49e9ef32c7cd3a3f7b
Author: Max Pedraza <[email protected]>

video: logo: allow the boot logo to come from the device tree

Add CONFIG_LOGO_DT_CLUT224, which makes fb_find_logo() look for a node
compatible with "linux,boot-logo-clut224" under /chosen before falling
back to the logos built into the kernel image.

> diff --git a/drivers/video/logo/logo.c b/drivers/video/logo/logo.c
> index 91535f8848da9..7f8b04ecfa987 100644
> --- a/drivers/video/logo/logo.c
> +++ b/drivers/video/logo/logo.c

[ ... ]

> +static int logo_dt_parse(struct device_node *np)
> +{
> +	unsigned int clutsize, npixels, i;
> +	unsigned char *clut, *data;
> +	u32 width, height;
> +	int len, ret;
> +
> +	ret = of_property_read_u32(np, "width", &width);
> +	if (ret)
> +		return ret;
> +
> +	ret = of_property_read_u32(np, "height", &height);
> +	if (ret)
> +		return ret;
> +
> +	if (!width || !height || (u64)width * height > LOGO_DT_MAX_PIXELS)
> +		return -EINVAL;
> +
> +	npixels = width * height;
> +
> +	len = of_property_count_u8_elems(np, "clut");
> +	if (len < 3 || len % 3)
> +		return -EINVAL;
> +
> +	clutsize = len / 3;
> +	if (clutsize > LOGO_DT_MAX_CLUT)
> +		return -EINVAL;
> +
> +	ret = of_property_count_u8_elems(np, "data");
> +	if (ret < 0)
> +		return ret;
> +	if ((unsigned int)ret != npixels)
> +		return -EINVAL;
> +
> +	clut = kmalloc(len, GFP_KERNEL);
> +	if (!clut)
> +		return -ENOMEM;
> +
> +	data = kmalloc(npixels, GFP_KERNEL);

[Severity: Medium]
Could this allocation trigger a kernel warning (WARN_ON_ONCE_GFP) and fail if
the device tree provides a valid boot logo larger than KMALLOC_MAX_SIZE?
LOGO_DT_MAX_PIXELS is 32MB, but kmalloc() without __GFP_NOWARN will warn if
it exceeds the maximum contiguous allocation size. Does this code need to use
kvmalloc() instead, or restrict the size limit to KMALLOC_MAX_SIZE?

> +	if (!data) {
> +		ret = -ENOMEM;
> +		goto err_free_clut;
> +	}
> +
> +	ret = of_property_read_u8_array(np, "clut", clut, len);

[ ... ]

> +static const struct linux_logo *logo_dt_find(void)
> +{
> +	static bool probed;
> +	struct device_node *np;
> +	int ret;
> +
> +	if (probed)
> +		return logo_dt_data ? &logo_dt_clut224 : NULL;
> +
> +	probed = true;
> +
> +	np = of_get_compatible_child(of_chosen, LOGO_DT_COMPATIBLE);

[Severity: Medium]
If the device tree lacks a /chosen node, of_chosen will be NULL. In that
scenario, wouldn't of_get_compatible_child() silently fall back to iterating
the children of the root node (of_root)? This seems to bypass the strict
constraint mentioned in the commit message that the node must be located under
/chosen.

> +	if (!np)
> +		return NULL;
> +
> +	if (of_device_is_available(np)) {
> +		ret = logo_dt_parse(np);
> +		if (ret)
> +			pr_warn("logo: ignoring malformed %pOF node (%d)\n",
> +				np, ret);
> +	}
> +
> +	of_node_put(np);

[ ... ]

> @@ -32,6 +184,7 @@ static bool logos_freed;
>  static int __init fb_logo_late_init(void)
>  {
>  	logos_freed = true;
> +	logo_dt_free();

[Severity: High]
Can this introduce a race condition resulting in a NULL pointer dereference
or use-after-free for framebuffer drivers that use async probing?

fb_logo_late_init() executes as a late_initcall_sync during do_initcalls(),
which happens before async_synchronize_full() guarantees async probes are
complete. If an async probe caches the logo pointer via fb_find_logo(), and
then fb_logo_late_init() frees the memory and clears the pointers, the async
probe might dereference the freed memory later in fb_show_logo().

>  	return 0;
>  }
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2