Re: [PATCH v5 03/13] drm: client: add splash client

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: out-of-bounds read and unaligned access in BMP blitting
- [High] drm: integer overflow in BGRT padding validation allows OOB write
--

commit 099d44ed94a2ad19b8702756f34790d1e8ae5693
Author: Francesco Valla <francesco@valla.it>

drm: client: add splash client

This commit adds a new DRM client to draw a simple splash screen during boot.
It supports displaying an EFI BGRT logo, a static BMP image loaded via
firmware, or a plain background color. The splash can be enabled via kernel
command line parameters.

> diff --git a/drivers/gpu/drm/clients/drm_splash.c b/drivers/gpu/drm/clients/drm_splash.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/gpu/drm/clients/drm_splash.c
[ ... ]
> +static void drm_splash_blit_pix16(struct iosys_map *map, unsigned int dpitch,
> +				  unsigned int x_pad, unsigned int y_pad,
> +				  const u8 *sbuf8, unsigned int spitch,
> +				  unsigned int width, unsigned int height,
> +				  bool invert_y, u32 format)
> +{
> +	unsigned int x, y, src_offset, dst_offset;
> +	u32 scolor, dcolor, wr_off;
> +
> +	for (y = 0; y < height; y++) {
> +		src_offset = (invert_y ? (height - y - 1) : y) * spitch;
> +		dst_offset = (y_pad + y) * dpitch;
> +
> +		for (x = 0; x < width; x++) {
> +			scolor = *(const u32 *)(&sbuf8[src_offset + 3 * x]);

[Severity: High]
Could this 32-bit cast cause memory safety and endianness issues?

The code casts a byte pointer to a 32-bit integer pointer to read a 3-byte
(24bpp) pixel. This introduces a few potential problems.

First, it performs unaligned memory accesses, which can trigger fatal
alignment exceptions on architectures that do not support hardware unaligned
loads, such as SPARC or some older ARM platforms.

Second, for the last pixel in an image row where there are no padding bytes,
the 4-byte read extends 1 byte beyond the end of the row. On the final row of
the image, this reads 1 byte past the end of the mapped memory buffer. This
could trigger a KASAN out-of-bounds read or a fatal page fault if the read
crosses an unmapped page boundary.

Finally, on big-endian architectures, reading the 3 bytes plus an arbitrary
4th byte directly into a u32 will scramble the internal BGR color channels,
resulting in corrupted colors.

Does this same cast pattern need to be addressed in drm_splash_blit_pix24()
and drm_splash_blit_pix32() below as well?

> +			dcolor = drm_draw_color_from_xrgb8888(scolor, format);
> +			wr_off = dst_offset + (x_pad + x) * sizeof(u16);
> +
> +			iosys_map_wr(map, wr_off, u16, dcolor);
> +		}
> +	}
> +}
[ ... ]
> +	if (splash->use_bgrt) {
> +		x_pad = efi_bgrt_image_offset_x();
> +		y_pad = efi_bgrt_image_offset_y();
> +
> +		if ((x_pad + bmp_width) > scanout->width ||
> +		    (y_pad + bmp_height) > scanout->height) {

[Severity: High]
Is this bounds check vulnerable to an integer overflow?

The x_pad and y_pad values are sourced directly from the ACPI BGRT table
via efi_bgrt_image_offset_x() and efi_bgrt_image_offset_y(), which return
32-bit unsigned integers. Because x_pad and scanout->width are unsigned int,
and bmp_width is a positive s32, the addition is evaluated as an unsigned
integer.

If an attacker provides an extremely large value for x_pad, such as
0xFFFFFFFF, in a compromised ACPI table, x_pad + bmp_width will wrap around
to a small number. This would bypass the greater-than scanout->width check.

Later during blitting, these wrapped offsets are used to calculate the write
offset. For example, calculating wr_off = dst_offset + (x_pad + x) * 3
would result in a massive out-of-bounds write and potential kernel memory
corruption.

Would it be safer to check these bounds using an overflow-safe method, or
perhaps by checking if x_pad > scanout->width - bmp_width?

> +			drm_err(client->dev, "splash: BGRT image would overflow");
> +			return -EINVAL;
> +		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013093243.505776-1-maximpedraza@gmail.com?part=3