[PATCH 1/1] spl: spl_legacy: simplify spl_parse_legacy_validate

[Marek Vasut]

On 7/22/23 22:37, Heinrich Schuchardt wrote:

Hi,

>>> diff --git a/common/spl/spl_legacy.c b/common/spl/spl_legacy.c
>>> index 095443c63d..9246f555e3 100644
>>> --- a/common/spl/spl_legacy.c
>>> +++ b/common/spl/spl_legacy.c
>>> @@ -22,10 +22,7 @@ static void spl_parse_legacy_validate(uintptr_t 
>>> start, uintptr_t size)
>>>       uintptr_t spl_end = (uintptr_t)_image_binary_end;
>>>       uintptr_t end = start + size;
>>> -    if ((start >= spl_start && start < spl_end) ||
>>> -        (end > spl_start && end <= spl_end) ||
>>> -        (start < spl_start && end >= spl_end) ||
>>> -        (start > end && end > spl_start))
>>> +    if (end > spl_start && start < spl_end)
>>>           panic("SPL: Image overlaps SPL\n");
>>>       if (size > CONFIG_SYS_BOOTM_LEN)
>>
>> Does this handle address space wrap around ? This:
>>
>> =====blob=====|..............|=====blob=====
>> .....|==spl==|..............................
>>
>> Hint: it does not.
> 
> The replaced code does not handle wrap around where end is below 
> spl_start. The following code would cover all cases of image wrap around:
> 
> +    if (end > spl_start && start < spl_end
> +        panic("SPL: Image overlaps SPL\n");
> +    if (start >= end)
> +        panic("SPL: Image wraps around\n");
> 
> I guess we don't have to test for spl_start >= spl_end.

I would really like someone to double-check this piece of code.

>>
>> +CC Rasmus.
>>
>> Look at all the other checks in U-Boot which do, and first factor them 
> 
> Which other instances do you refer to?

See e.g. bootm_find_images() in boot/bootm.c .
Basically git grep -i overlap and it shows a few more sites.

> The lmb library does not to check for wrap around but the wrap around 
> checks would have to be in other places than lmb_addrs_overlap().

I am kind-of tempted to suggest we should add LMB to SPL to fix this 
properly, since that way LMB would cover all vital parts of U-Boot and 
assure they won't be overwritten, not just code.