Re: [PATCH 1/2] video: bmp: bound RLE8 decode writes to the framebuffer

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