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. > } > > + 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. > Or not have + 1 for dir_len and calloc with + 1. Regards, -- Köry Maincent, Bootlin Embedded Linux and kernel engineering https://bootlin.com
