Re: [PATCH v2 4/6] dt-bindings: display: allow the boot logo in a reserved memory region

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Arbitrary boot logo placement via device tree allows out-of-bounds memory writes in framebuffer drivers.
- [Medium] The YAML schema enforces contradictory constraints by unconditionally requiring inline image properties, making the new `memory-region` property impossible to use without failing validation.
- [Medium] The kernel driver lacks any C implementation to parse or map the newly added `memory-region` property.
- [Medium] Using `kmalloc` for potentially large image allocations will fail for sizes exceeding `KMALLOC_MAX_SIZE`.
--

--- Patch [4]: [PATCH v2 4/6] dt-bindings: display: allow the boot logo in a reserved memory region ---
commit 53263fda0e15a2f2d945eb9940e2236f6120727d
Author: Max Pedraza <[email protected]>

dt-bindings: display: allow the boot logo in a reserved memory region

This commit extends the boot logo device tree binding to support loading
the image from a reserved memory region. It adds a memory-region property
as a mutually exclusive alternative to providing the image data inline.

> Carrying the image in the device tree ties it to the device tree, but the
> image and where it goes on screen are independent axes of variation. One
> board sold to several customers wants several device trees that differ in
> the logo; one customer with several products built on that board wants the
> same logo placed differently on each panel. The second case would otherwise
> mean duplicating the same image into every device tree.

[Severity: Medium]
Can allocating these large images fail?  Using kmalloc() for potentially
large image allocations up to the 32MB LOGO_DT_MAX_PIXELS limit will fail
for sizes exceeding KMALLOC_MAX_SIZE:

drivers/video/logo/logo.c:logo_dt_parse() {
	...
	data = kmalloc(npixels, GFP_KERNEL);
	if (!data) {
		ret = -ENOMEM;
		goto err_free_clut;
	}
	...
}

> 
> Let the node point at a reserved memory region filled in by the bootloader
> instead, so one image can be shared by device trees that differ only in
> placement. The region starts with a small header carrying a magic number
> and the geometry, so the kernel can tell a logo from an empty or stale
> region and bounds check everything against the reservation.

[Severity: Medium]
Is the parsing logic for this feature missing from this patch series?  The
kernel driver lacks any C implementation to parse or map the newly added
memory-region property.

In drivers/video/logo/logo.c:logo_dt_parse(), it unconditionally checks
for the inline properties and returns an error if they are absent, with no
code to handle the memory-region property:

drivers/video/logo/logo.c:logo_dt_parse() {
	...
	ret = of_property_read_u32(np, "width", &width);
	if (ret)
		return ret;
	...
}

[Severity: High]
Are the placement coordinates actually bounds-checked?  Arbitrary boot
logo placement via device tree allows out-of-bounds memory writes in
framebuffer drivers.

In fb_logo_dt_place(), unvalidated coordinates from the DT are assigned
to image->dy:

drivers/video/fbdev/core/fb_logo.c:fb_logo_dt_place() {
	...
	image->dx = max(dx + p->offset_x, 0);
	image->dy = fb_logo_dt_top(info->var.yres, image->height);
}

In fb_do_show_logo(), the bounds checks are completely missing for the Y
axis depending on rotation:

drivers/video/fbdev/core/fb_logo.c:fb_do_show_logo() {
	...
	if (rotate == FB_ROTATE_UR) {
		for (x = 0;
		     x < num && image->dx + image->width <= info->var.xres;
		     x++) {
			info->fbops->fb_imageblit(info, image);
	...
}

When fb_imageblit() is called, the driver computes an address using dy,
which will write far outside the framebuffer limits.

[ ... ]

> diff --git a/Documentation/devicetree/bindings/display/linux,boot-logo-clut224.yaml b/Documentation/devicetree/bindings/display/linux,boot-logo-clut224.yaml
> index a6a2069648359..7aec0cc2d3776 100644
> --- a/Documentation/devicetree/bindings/display/linux,boot-logo-clut224.yaml
> +++ b/Documentation/devicetree/bindings/display/linux,boot-logo-clut224.yaml
> @@ -59,6 +59,23 @@ properties:
>        index into the colour lookup table. The property length must be equal to
>        width multiplied by height.
>  
> +  memory-region:
> +    maxItems: 1

[Severity: Medium]
Will this property always fail validation?  The top-level required list
permanently mandates width, height, clut, and data.  If a device
tree node is added using only memory-region, it fails the global
required check:

Documentation/devicetree/bindings/display/linux,boot-logo-clut224.yaml:
required:
  - compatible
  - width
  - height
  - clut
  - data

If a node provides both, it fails the oneOf constraint since the subschemas
become mutually exclusive.

[ ... ]

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