From 3704b888a4cabac8daea20a4504d513bc47153ca Mon Sep 17 00:00:00 2001 From: Mikhail Kshevetskiy Date: Tue, 10 Jun 2025 12:56:30 +0300 Subject: [PATCH 1/3] common/spl: fix potential out of buffer access in spl_fit_get_image_name function 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 Reviewed-by: Tom Rini --- 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)) { From 3eb43c54fadba457f22e415a2821145164efe662 Mon Sep 17 00:00:00 2001 From: Mikhail Kshevetskiy Date: Tue, 10 Jun 2025 12:56:31 +0300 Subject: [PATCH 2/3] common/spl: handle properly images with bad checksum load_simple_fit() returns -EPERM for the images with broken signatures. Unfortunately this may conflict with image loaging selection on the base of boot phase. See commit 873112db9ce68c38984ff25808dde726f8dd5573 ("spl: Support selecting images based on phase in simple FIT"). Thus loading of configurations { uboot { description = "u-boot"; firmware = "atf"; loadables = "atf", "tee", "uboot"; }; }; with damaged "tee" image may finish without errors. This may results in board bricking. This patch fixes commit 873112db9ce68c38984ff25808dde726f8dd5573 ("spl: Support selecting images based on phase in simple FIT") by replacing EPERM with EBADSLT places where it should be done. Signed-off-by: Mikhail Kshevetskiy --- common/spl/spl_fit.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index ab277bb2baa..321954a1547 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -200,7 +200,7 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, * the image gets loaded to the address pointed to by the * load_addr member in this struct, if load_addr is not 0 * - * Return: 0 on success, -EPERM if this image is not the correct phase + * Return: 0 on success, -EBADSLT if this image is not the correct phase * (for CONFIG_BOOTMETH_VBE_SIMPLE_FW), or another negative error number on * other error. */ @@ -236,7 +236,7 @@ static int load_simple_fit(struct spl_load_info *info, ulong fit_offset, return ret; } else { log_debug("- phase mismatch, skipping this image\n"); - return -EPERM; + return -EBADSLT; } } @@ -475,7 +475,7 @@ static int spl_fit_append_fdt(struct spl_image_info *spl_image, image_info.load_addr = (ulong)tmpbuffer; ret = load_simple_fit(info, offset, ctx, node, &image_info); - if (ret == -EPERM) + if (ret == -EBADSLT) continue; else if (ret < 0) break; @@ -835,7 +835,7 @@ int spl_load_simple_fit(struct spl_image_info *spl_image, image_info.load_addr = 0; ret = load_simple_fit(info, offset, &ctx, node, &image_info); - if (ret < 0 && ret != -EPERM) { + if (ret < 0 && ret != -EBADSLT) { printf("%s: can't load image loadables index %d (ret = %d)\n", __func__, index, ret); return ret; From 8bb9c275c484206c0314014d8215770aaac4cefe Mon Sep 17 00:00:00 2001 From: Mikhail Kshevetskiy Date: Tue, 10 Jun 2025 12:56:32 +0300 Subject: [PATCH 3/3] common/spl: improve error handling in spl_fit This fix a possible NULL pointer dereference. There is also a risk of memory leaking within the same portion of code. The leak will happen if loaded image is bad or damaged. In this case u-boot-spl will try booting from the other available media. Unfortunately resources allocated for previous boot media will NOT be freed. We can't fix that issue as the memory allocation mechanism used here is unknown. It can be different kinds of malloc() or something else. To somewhat reduce memory consumption, one can try to reuse previously allocated memory as it's done in board_spl_fit_buffer_addr() from test/image/spl_load.c. The corresponding comment was put to the code as well. Signed-off-by: Mikhail Kshevetskiy Reviewed-by: Anshul Dalal --- common/spl/spl_fit.c | 40 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 321954a1547..b3824af475f 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -703,13 +703,51 @@ static int spl_simple_fit_read(struct spl_fit_info *ctx, */ size = get_aligned_image_size(info, size, 0); buf = board_spl_fit_buffer_addr(size, size, 1); + if (!buf) { + /* + * We assume that none of the board will ever use 0x0 as a + * valid load address. Theoretically some board could use it, + * but this is extremely unlikely. + */ + return -EIO; + } count = info->read(info, offset, size, buf); + if (!count) { + /* + * FIT could not be read. This means we should free the + * memory allocated by board_spl_fit_buffer_addr(). + * Unfortunately, we don't know what memory allocation + * mechanism was used: + * - For the SPL_SYS_MALLOC_SIMPLE case nothing could + * be done. The memory just could not be freed. + * - For statically allocated memory buffer we can try + * to reuse previously allocated memory (example: + * board_spl_fit_buffer_addr() function from the + * file test/image/spl_load.c). + * - For normall malloc() -- memory leak can't be easily + * avoided. To somehow reduce memory consumption the + * next calls of board_spl_fit_buffer_addr() could + * reallocate previously allocated buffer and use + * them again. This is somethat similar to the approach + * used for statically allocated buffer. + * + * Please note: + * - FIT images with data placed outside of the FIT + * structure will cause small memory leak (several + * kilobytes), + * - FIT images with data placed inside to the FIT + * structure may cause huge memory leak (up to + * several megabytes). Do NOT use such images! + */ + return -EIO; + } + ctx->fit = buf; debug("fit read offset %lx, size=%lu, dst=%p, count=%lu\n", offset, size, buf, count); - return (count == 0) ? -EIO : 0; + return 0; } static int spl_simple_fit_parse(struct spl_fit_info *ctx)