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
