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

[Tom Rini]

On Wed, Jun 25, 2025 at 04:30:06PM +0300, Mikhail Kshevetskiy wrote:
> 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.

Yes, I'll pick up Heinrich's series soon to resolve this part of things.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20250625/2bb40f07/attachment.sig>