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
