On 11/12/25 3:22 PM, Kory Maincent wrote:
On Wed, 12 Nov 2025 14:44:33 +0100
Quentin Schulz <[email protected]> wrote:
Hi Köry,
On 11/12/25 2:17 PM, Kory Maincent wrote:
From: "Kory Maincent (TI.com)" <[email protected]>
Fix two memory allocation bugs in label_boot_extension():
1. When label->fdtdir is not set, overlay_dir was used without any
memory allocation. Add the missing calloc() in the else branch.
2. When label->fdtdir is set, the allocation incorrectly used the
'len' variable instead of 'dir_len'. The 'dir_len' variable is
calculated to include the fdtdir length plus the trailing slash,
while 'len' was only for the fdtdir length. This caused incorrect
memory allocation size.
These issues could lead to memory corruption or undefined behavior when
processing device tree overlays via PXE boot.
Closes: https://lists.denx.de/pipermail/u-boot/2025-November/602892.html
Fixes: 935109cd9e97 ("boot: pxe_utils: Add extension board devicetree
overlay support") Signed-off-by: Kory Maincent (TI.com)
<[email protected]> ---
boot/pxe_utils.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 038416203fc..7a64b6b97d4 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -474,7 +474,7 @@ static void label_boot_extension(struct pxe_context
*ctx, slash = "";
dir_len = strlen(label->fdtdir) + strlen(slash) + 1;
- overlay_dir = calloc(1, len);
+ overlay_dir = calloc(1, dir_len);
if (!overlay_dir)
return;
@@ -482,6 +482,10 @@ static void label_boot_extension(struct pxe_context
*ctx, slash);
} else {
dir_len = 2;
+ overlay_dir = calloc(1, dir_len);
+ if (!overlay_dir)
+ return;
+
snprintf(overlay_dir, dir_len, "/");
}
I'm wondering if we couldn't make this easier to maintain by not having
two calloc and snprintf calls?
diff --git a/boot/pxe_utils.c b/boot/pxe_utils.c
index 038416203fc..6c1bf05cf66 100644
--- a/boot/pxe_utils.c
+++ b/boot/pxe_utils.c
@@ -474,17 +474,17 @@ static void label_boot_extension(struct
pxe_context *ctx,
slash = "";
dir_len = strlen(label->fdtdir) + strlen(slash) + 1;
- overlay_dir = calloc(1, len);
- if (!overlay_dir)
- return;
-
- snprintf(overlay_dir, dir_len, "%s%s", label->fdtdir,
- slash);
} else {
dir_len = 2;
- snprintf(overlay_dir, dir_len, "/");
+ slash = "/";
In fact I think "./" should be the default one here.
It currently is "/" so that would be a third change/fix (to be explained
in the commit log at least; maybe in separate commits).
}
+ overlay_dir = calloc(1, dir_len);
+ if (!overlay_dir)
+ return;
+
+ snprintf(overlay_dir, dir_len, "%s%s", label->fdtdir?: "", slash);
Yes, that's a good idea!
Also, we probably want dir_len = len + strlen(slash) + 1 to avoid a
second strlen on label->fdtdir (at the top of the git context here).
Indeed!
Finally, I'm wondering if the snprintf should not be dir_len - 1
considering we calloc with enough room for the trailing NUL character?
No, snprintf second argument is the max size written including the null
character.
More specifically, it's going to write the NUL character even if not
present in the input string(s), at the last index, which is something I
had missed while reading the CPP doc of snprintf (the manpage is a bit
less clear about that). So never mind about this suggestion :)
Cheers,
Quentin
