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 mon...@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.g...@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.kshevets...@iopsys.eu> > >>> Reviewed-by: Tom Rini <tr...@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
signature.asc
Description: PGP signature
