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

Hi Simon,

Thanks! Fixed in v3.

And good catch on the unbounded RLE reads. I'll send that as a separate 
follow-up.


Thanks,
Shah

On 28.07.26 13:54, Simon Glass wrote:
> 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