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
