[U-Boot] [PATCH v5 01/15] video: bmp: check resolutions of panel/bitmap

[Heinrich Schuchardt]

On 10/25/19 9:43 AM, Patrice CHOTARD wrote:
> Hi Heinrich
>
> On 10/24/19 10:43 PM, Heinrich Schuchardt wrote:
>> On 10/24/19 4:05 PM, Patrice CHOTARD wrote:
>>> Hi Heinrich, all
>>>
>>> On 10/7/19 7:34 PM, Heinrich Schuchardt wrote:
>>>> On 10/7/19 3:29 PM, Yannick Fertré wrote:
>>>>> If the size of the bitmap is bigger than the size of
>>>>> the panel then errors appear when calculating axis alignment
>>>>> and the copy of bitmap is done outside of framebuffer.
>>>>>
>>>>> Signed-off-by: Yannick Fertré <yannick.fertre at st.com>
>>>>> ---
>>>>>    drivers/video/video_bmp.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/video/video_bmp.c b/drivers/video/video_bmp.c
>>>>> index 193f37d..4af1fb4 100644
>>>>> --- a/drivers/video/video_bmp.c
>>>>> +++ b/drivers/video/video_bmp.c
>>>>> @@ -249,6 +249,13 @@ int video_bmp_display(struct udevice *dev, ulong bmp_image, int x, int y,
>>>>>
>>>>>        padded_width = (width & 0x3 ? (width & ~0x3) + 4 : width);
>>>>>
>>>>> +    /* check if picture size exceeds panel size */
>>>>> +    if ((pwidth < width) || (priv->ysize < height)) {
>>>>> +        printf("Error: BMP size %d x %d is bigger than panel size %d x %d\n",
>>>>> +               (int)width, (int)height, priv->xsize, priv->ysize);
>>>>> +        return -EINVAL;
>>>>> +    }
>>>>> +
>>>>>        if (align) {
>>>>>            video_splash_align_axis(&x, priv->xsize, width);
>>>>>            video_splash_align_axis(&y, priv->ysize, height);
>>>>
>>>> This is followed by:
>>>>
>>>>          if ((x + width) > pwidth)
>>>>                  width = pwidth - x;
>>>>          if ((y + height) > priv->ysize)
>>>>                  height = priv->ysize - y;
>>>>
>>>> These lines will clip a bitmap that given the left upper corner x, y
>>>> does not fit onto the screen.
>>>>
>>>> So isn't this patch superfluous?
>>>>
>>>> What is missing are checks for x and y.
>>>>
>>>> For testing I have used qemu_x86 and added
>>>>
>>>>      #define CONFIG_BMP_24BPP y
>>>>
>>>> to the top of drivers/video/video_bmp.c and loaded a 24bit bitmap.
>>>>
>>>> Clipping works as expected. But passing a value of x exceeding the
>>>> screen width lead to a crash.
>>>>
>>>> What I really dislike in the existing coding is that CONFIG_BMP_*BPP is
>>>> defined in includes instead of using Kconfig but that is a different story.
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>
>>> For information all this series patches have been applied on u-boot/master excepts  this one.
>>>
>>> Unfortunately, without this patch, stm32f746-discovery board doesn't boot anymore.
>>
>> I still do not understand why this patch is needed.
>>
>> Could you, please, try to analyze why the board does not boot.
>>
>> What is wrong with the existing code for clipping?
>> Or is the non-booting just coincidence but the bug is somewhere else?
>>
>> What are the values of the parameters passed to video_bmp_display()?
>> Which bitmap file are you using?
>> What is the size of the display?
>
>
> To sum-up, on all STM32 boards, the same BMP splashcreen is used.
>
> In the particular case of STM32F746-Disco board, the panel size is smaller then the BMP size.
>
> So, some size check must be done to avoid overflow when writing inside the framebuffer.

That is why we have lines to clip images:

          if ((x + width) > pwidth)
                  width = pwidth - x;
          if ((y + height) > priv->ysize)
                  height = priv->ysize - y;

Why is this not working in you case?

Best regards

Heinrich


>
> If needed, Yannick, which is multimedia expert, can give you more precise details.
>
> Thanks
>
> Patrice
>
>
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Heinrich, could this patch be merged, waiting for a clean patch from Yannick ?
>>>
>>> Regards
>>>
>>> Patrice
>>>
>>
>