[BUG] [PATCH v5 1/3] common/spl: fix potential out of buffer access in spl_fit_get_image_name function

[Mikhail Kshevetskiy]

I think I can fix this patch or the patchset of Heinrich Schuchardt
could be applied

Mikhail Kshevetskiy

On 25.06.2025 16:27, Michal Simek wrote:
> [You don't often get email from monstr at monstr.eu. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Tom,
>
> út 24. 6. 2025 v 14:59 odesílatel Heinrich Schuchardt
> <xypron.glpk at gmx.de> napsal:
>> On 10.06.25 11:56, Mikhail Kshevetskiy wrote:
>>> The current code have two issues:
>>> 1) ineffective NULL pointer check
>>>
>>>       str = strchr(str, '\0') + 1
>>>       if (!str || ...
>>>
>>>     The str here will never be NULL (because we add 1 to result of strchr())
>>>
>>> 2) strchr() may go out of the buffer for the special forms of name variable.
>>>     It's better use memchr() function here.
>>>
>>>     According to the code the property is a sequence of C-string like
>>>     shown below:
>>>
>>>       'h', 'e', 'l', 'l', 'o', '\0', 'w', 'o', 'r', 'l', 'd', '\0', '!', '\0'
>>>
>>>     index is the string number we are interested, so
>>>
>>>       index = 0   =>  "hello",
>>>       index = 1   =>  "world",
>>>       index = 2   =>  "!"
>>>
>>>     The issue will arrise if last string for some reason have no terminating
>>>     '\0' character. This can happen for damaged or specially crafted dtb.
>>>
>>> Signed-off-by: Mikhail Kshevetskiy <mikhail.kshevetskiy at iopsys.eu>
>>> Reviewed-by: Tom Rini <trini at konsulko.com>
>>> ---
>>>   common/spl/spl_fit.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>>> index 86506d6905c..ab277bb2baa 100644
>>> --- a/common/spl/spl_fit.c
>>> +++ b/common/spl/spl_fit.c
>>> @@ -86,11 +86,12 @@ static int spl_fit_get_image_name(const struct spl_fit_info *ctx,
>>>
>>>       str = name;
>>>       for (i = 0; i < index; i++) {
>>> -             str = strchr(str, '\0') + 1;
>>> -             if (!str || (str - name >= len)) {
>>> +             str = memchr(str, '\0', name + len - str);
>>> +             if (!str) {
>>>                       found = false;
>>>                       break;
>>>               }
>>> +             str++;
>>>       }
>>>
>>>       if (!found && CONFIG_IS_ENABLED(SYSINFO) && !sysinfo_get(&sysinfo)) {
>> Since this patch (commit 3704b888a4cabac8) on the Milk-V Mars board I
>> see a message "cannot find image node ''" and on some builds I have seen
>> boot failures.
>>
>> With your patch and some debug messages:
>>
>> U-Boot SPL 2025.07-rc4-00017-g3704b888a4ca-dirty (Jun 24 2025 - 14:45:11
>> +0200)
>> DDR version: dc2e84f0.
>> Trying to boot from MMC2
>> spl_fit_get_image_name(): index = 0, name = 'opensbi' len = 8
>> spl_fit_get_image_name(): outname = 'opensbi'
>> spl_fit_get_image_name(): index = 0, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = 'uboot'
>> spl_fit_get_image_name(): index = 0, name = 'fdt-2' len = 6
>> spl_fit_get_image_name(): outname = 'fdt-2'
>> spl_fit_get_image_name(): index = 0, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = 'uboot'
>> spl_fit_get_image_name(): index = 0, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = 'uboot'
>> spl_fit_get_image_name(): index = 1, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = ''
>> cannot find image node '': -1
>>
>>
>> Before that patch:
>>
>> U-Boot SPL 2025.07-rc4-00017-g3704b888a4ca-dirty (Jun 24 2025 - 14:34:50
>> +0200)
>> DDR version: dc2e84f0.
>> Trying to boot from MMC2
>> spl_fit_get_image_name(): index = 0, name = 'opensbi' len = 8
>> spl_fit_get_image_name(): outname = 'opensbi'
>> spl_fit_get_image_name(): index = 0, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = 'uboot'
>> spl_fit_get_image_name(): index = 0, name = 'fdt-2' len = 6
>> spl_fit_get_image_name(): outname = 'fdt-2'
>> spl_fit_get_image_name(): index = 0, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = 'uboot'
>> spl_fit_get_image_name(): index = 0, name = 'uboot' len = 6
>> spl_fit_get_image_name(): outname = 'uboot'
>> spl_fit_get_image_name(): index = 1, name = 'uboot' len = 6
>> no string for index 1
>>
>>
>> With your patch spl_fit_get_image_name() does not detect that there is
>> no property at index 1 and does not return -E2BIG. Instead it signals
>> the property is an empty string and returns 0 which is not expected.
>>
> I also see random chars printed on the console on SOM with SPL flow.
> This patch should be reverted.
>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: http://www.monstr.eu/ p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs