Hi shj,
On 2026-07-22T07:10:10, shj <[email protected]> wrote:
> video: bmp: bound RLE8 decode writes to the framebuffer
>
> video_display_rle8_bitmap() moves the framebuffer cursor fb up by a
> full row plus a scanline on each End-Of-Line escape, with no
> lower-bound check. Repeated EOL escapes desynchronise fb from the
> scanline index y: after height - 1 escapes y is back in range while fb
> has drifted about one framebuffer below priv->fb, and the decoder
> writes pixel data before the start of the framebuffer. The DELTA
> escape recomputes fb from an unchecked y as well. A crafted RLE8
> image displayed from the splash-screen or PXE-menu path can therefore
> write out of bounds.
>
> Check the cursor against [priv->fb, priv->fb + fb_size) before each
> run and reject the image with -EINVAL if a write would fall outside
> it.
>
> Signed-off-by: shj <[email protected]>
>
> drivers/video/video_bmp.c | 26 +++++++++++++++++++-------
> 1 file changed, 19 insertions(+), 7 deletions(-)
> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c
> @@ -337,8 +347,10 @@ int video_bmp_display(struct udevice *dev, ulong
> bmp_image, int x, int y,
> - video_display_rle8_bitmap(dev, bmp, bpix,
> palette, fb,
> - x, y, width, height);
> + if (video_display_rle8_bitmap(dev, bmp, bpix,
> + palette, fb, x, y,
> + width, height))
> + return -EINVAL;
This now returns a proper error code, so please propagate it rather
than flattening every non-zero value to -EINVAL:
int ret;
...
ret = video_display_rle8_bitmap(dev, bmp, bpix, palette, fb,
x, y, width, height);
if (ret)
return ret;
With this:
Reviewed-by: Simon Glass <[email protected]>
> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c
> @@ -176,6 +178,9 @@ static void video_display_rle8_bitmap(struct udevice *dev,
> + if (fb < fb_start ||
> + fb + cnt * bytes_per_pixel
> > fb_end)
> + return -EINVAL;
BTW this bounds the writes correctly, but the reads from the RLE
stream (bmap) are still unbounded: the decoder advances bmap and the
encoded-run aggregation reads bmap[2]/bmap[3] with no check against
the end of the loaded image, so a crafted image from the same
splash/PXE path can still drive an out-of-bounds read. That is beyond
the scope of this patch, so a follow-up is fine.
Regards,
Simon
