mtd: Avoid boot crash in RedBoot partition table parser

PATCH Superseded Finn Thain 2 participants 2 messages Feb 15 – Feb 16

Subsystems (2)

Stable branch Memory technology devices (MTD)

lore

Share & tools

lore

Finn Thain

author

2026-02-15 00:22 UTC #0 lore raw

PATCH 1/1 mtd: Avoid boot crash in RedBoot partition table parser

Given CONFIG_FORTIFY_SOURCE=y, and given a recent compiler, commit 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available") produces the warning below and an oops.

    Searching for RedBoot partition table in 50000000.flash at offset 0x7e0000
    ------------[ cut here ]------------
    WARNING: lib/string_helpers.c:1035 at 0xc029e04c, CPU#0: swapper/0/1
    memcmp: detected buffer overflow: 15 byte read of buffer size 14
    Modules linked in:
    CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.19.0 #1 NONE

I couldn't see how memcmp() exceeds the buffer here, so the simplest way to prevent the regression was to perform memcmp() on the original name rather than the copy.

Cc [email protected]

Fixes 439a1bcac648 ("fortify: Use __builtin_dynamic_object_size() when available")

Signed-off-by Finn Thain <[email protected]>

---
I put commit 439a1bcac648 into a Fixes tag because git bisect identified that commit as the source of the regression. But I don't know anything about __builtin_dynamic_object_size() or its limitations. So perhaps the real bug lies elsewhere. The compiler I'm using is this one:

$ armeb-softfloat-linux-musleabi-gcc --version
armeb-softfloat-linux-musleabi-gcc (Gentoo Hardened 13.4.1_p20250807 p8) 13.4.1 20250807
---
 drivers/mtd/parsers/redboot.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/mtd/parsers/redboot.c +5 -5

index 3b55b676ca6b..6e253f6c45c9 100644
@@ -269,14 +269,14 @@ static int parse_redboot_partitions(struct mtd_info *master,
 		parts[i].name = names;
 
 		strcpy(names, fl->img->name);
+		names += strlen(names) + 1;
+
 #ifdef CONFIG_MTD_REDBOOT_PARTS_READONLY
-		if (!memcmp(names, "RedBoot", 8) ||
-		    !memcmp(names, "RedBoot config", 15) ||
-		    !memcmp(names, "FIS directory", 14)) {
+		if (!memcmp(fl->img->name, "RedBoot", 8) ||
+		    !memcmp(fl->img->name, "RedBoot config", 15) ||
+		    !memcmp(fl->img->name, "FIS directory", 14))
 			parts[i].mask_flags = MTD_WRITEABLE;
-		}
 #endif
-		names += strlen(names) + 1;
 
 #ifdef CONFIG_MTD_REDBOOT_PARTS_UNALLOCATED
 		if (fl->next && fl->img->flash_base + fl->img->size + master->erasesize <= fl->next->img->flash_base) {

Signature · 1 line

2.49.1

Kees Cook

2026-02-16 03:48 UTC #1 lore raw

Review

Quoted 3 lines · Finn Thain

memcmp: detected buffer overflow: 15 byte read of buffer size 14
[...]
(quoted diff) +0 -1
-		    !memcmp(names, "RedBoot config", 15) ||

The warning is saying that "names" is detected to be 14 bytes in size. It is allocated here, and nulllen can be ignored (it is fixed size, either 0 or sizeof(nullstring), and skipped over):

        parts = kzalloc(sizeof(*parts) * nrparts + nulllen + namelen, GFP_KERNEL);
	...
        nullname = (char *)&parts[nrparts];
	...
        names = nullname + nulllen;

so "names" is pointing to the final "namelen" many bytes of the allocation. Calculating "namelen" happens via an earlier for loop:

        buf = vmalloc(master->erasesize);
	...
        ret = mtd_read(master, offset, master->erasesize, &retlen,
                       (void *)buf);
	...
        numslots = (master->erasesize / sizeof(struct fis_image_desc));
	...
        for (i = 0; i < numslots; i++) {
		...
                namelen += strlen(buf[i].name) + 1;

So namelen could be basically any length at all. This fortify warning looks legit to me -- this code used to be reading beyond the end of the allocation.

Your patch looks technically correct, but why not just use strcmp? Both arguments are NUL-terminated. The memcmp() calls were all including the NUL byte, so they're effectively doing strcmp except that they weren't stopping at the first NUL byte. So probably just better to do:

drivers/mtd/parsers/redboot.c +3 -3

index 3b55b676ca6b..c06ba7a2a34b 100644
@@ -270,9 +270,9 @@ static int parse_redboot_partitions(struct mtd_info *master,
 
 		strcpy(names, fl->img->name);
 #ifdef CONFIG_MTD_REDBOOT_PARTS_READONLY
-		if (!memcmp(names, "RedBoot", 8) ||
-		    !memcmp(names, "RedBoot config", 15) ||
-		    !memcmp(names, "FIS directory", 14)) {
+		if (!strcmp(names, "RedBoot") ||
+		    !strcmp(names, "RedBoot config") ||
+		    !strcmp(names, "FIS directory")) {
 			parts[i].mask_flags = MTD_WRITEABLE;
 		}
 #endif

Signature · 1 line

Kees Cook