Re: [PATCH v2 3/6] fbdev: honour the device tree boot logo placement properties

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unvalidated device tree coordinates cause out-of-bounds memory accesses in the framebuffer.
- [High] DT logo placement logic ignores SMP logo count and extra logos, breaking centering and overlapping extra logos.
- [High] fb_logo_dt_reserve() calculates logical console space reservations using the incorrect physical axis during screen rotation.
- [High] Overriding logo rotation via device tree breaks the reservation coordinate space contract with fbcon.
- [Medium] TOC-TOU initialization race on `read_done` static flag in `fb_logo_dt_read`.
--

--- Patch [3]: [PATCH v2 3/6] fbdev: honour the device tree boot logo placement properties ---
commit bd67300c4f145f9b35c28ceac495f14e402f271d
Author: Max Pedraza <[email protected]>

fbdev: honour the device tree boot logo placement properties

A logo supplied by the device tree describes a specific product, so where
it lands on the panel matters in a way it does not for the built-in Tux.
Honour the optional "logo-position", "logo-offset", "logo-centered" and
"logo-rotation" properties of the "linux,boot-logo-clut224" node.

> diff --git a/drivers/video/fbdev/core/fb_logo.c b/drivers/video/fbdev/core/fb_logo.c
> --- a/drivers/video/fbdev/core/fb_logo.c
> +++ b/drivers/video/fbdev/core/fb_logo.c
> @@ -2,12 +2,178 @@
>  
>  #include <linux/fb.h>
>  #include <linux/linux_logo.h>
> +#include <linux/of.h>

[ ... ]

> +static void fb_logo_dt_read(void)
> +{
> +	struct fb_logo_dt_placement *p = &fb_logo_dt;
> +	static bool read_done;
> +	struct device_node *np;
> +	const char *rotation;
> +	u32 val[2];
> +	int rot;
> +
> +	if (read_done)
> +		return;
> +
> +	read_done = true;
> +
> +	np = of_find_compatible_node(NULL, NULL, "linux,boot-logo-clut224");

[Severity: Medium]
Could this lead to a race condition if called concurrently?

By setting read_done to true before the fb_logo_dt structure is actually
populated, could another thread see read_done as true and proceed to use an
uninitialized structure?

[ ... ]

> +/* Top edge of the logo, in the coordinate space the caller works in */
> +static int fb_logo_dt_top(unsigned int yres, unsigned int logo_height)
> +{
> +	struct fb_logo_dt_placement *p = &fb_logo_dt;
> +	int top;
> +
> +	if (p->centered)
> +		top = ((int)yres - (int)logo_height) / 2;
> +	else if (p->has_position)
> +		top = p->y;
> +	else
> +		top = 0;
> +
> +	return max(top + p->offset_y, 0);
> +}

[Severity: High]
When the screen is hardware-rotated (for example, CW or CCW), does returning
the physical Y coordinate here result in a broken reservation geometry?

fb_prepare_logo() needs the reservation height in the logical coordinate space
of the console. When rotated, the console's logical vertical axis maps to the
hardware's horizontal X-axis, but this logic unconditionally uses the physical
Y position (p->y).

> +static void fb_logo_dt_place(struct fb_info *info, struct fb_image *image)
> +{
> +	struct fb_logo_dt_placement *p = &fb_logo_dt;
> +	int dx;
> +
> +	fb_logo_dt_read();
> +
> +	if (!p->valid)
> +		return;
> +
> +	if (p->centered)
> +		dx = ((int)info->var.xres - (int)image->width) / 2;
> +	else if (p->has_position)
> +		dx = p->x;
> +	else
> +		dx = image->dx;
> +
> +	image->dx = max(dx + p->offset_x, 0);
> +	image->dy = fb_logo_dt_top(info->var.yres, image->height);
> +}

[Severity: High]
Does this logic break the centering of SMP logos and the vertical stacking of
extra logos?

When rendering multiple logos, fb_show_logo_line() calculates a block-centered
X coordinate for the SMP penguins and an accumulated Y coordinate to stack any
extra logos.

By calculating dx using only a single image->width, this seems to break SMP
block centering. Additionally, overwriting image->dy with a static position
forces all subsequent extra logos to render at the exact same vertical position
causing them to overlap.

[Severity: High]
Can these unvalidated device tree coordinates cause an out-of-bounds memory
access in the framebuffer?

fb_logo_dt_place() applies the device tree positions directly to image->dx
and image->dy without clamping them to the actual screen dimensions.

If a large offset is provided, would fb_do_show_logo() process the unvalidated
Y coordinate? For example, in the FB_ROTATE_UR case:

    for (x = 0;
         x < num && image->dx + image->width <= info->var.xres;
         x++) {
            info->fbops->fb_imageblit(info, image);

The loop checks the X bounds but lacks checks for image->dy + image->height
against info->var.yres, potentially passing arbitrarily large Y coordinates
to the driver's fb_imageblit callback.

[ ... ]

> @@ -423,6 +592,8 @@ int fb_prepare_logo(struct fb_info *info, int rotate)
>  
>  	memset(&fb_logo, 0, sizeof(struct logo_data));
>  
> +	rotate = fb_logo_dt_rotation(rotate);
> +
>  	if (info->flags & FBINFO_MISC_TILEBLITTING ||
>  	    info->fbops->owner || !fb_logo_count)
>  		return 0;

[Severity: High]
Does overriding the caller's rotation parameter here break the coordinate space
agreement with the fbcon subsystem?

The fbcon subsystem calls fb_prepare_logo() with its own console rotation state
and expects the returned reservation height to scale accurately against its
logical vertical axis. If fb_logo_dt_rotation() overrides this with a
differing rotation from the device tree, will fbcon incorrectly interpret the
resulting reservation height against its differing logical axis?

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