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

[Heinrich Schuchardt]

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.

Best regards

Heinrich