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

[Michal Simek]

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: 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