[PATCH] boot: pxe_utils: Fix memory allocation issues in overlay_dir handling

[Quentin Schulz]

On 11/12/25 3:22 PM, Kory Maincent wrote:
> On Wed, 12 Nov 2025 14:44:33 +0100
> Quentin Schulz <quentin.schulz at cherry.de> wrote:
> 
>> Hi Köry,
>>
>> On 11/12/25 2:17 PM, Kory Maincent wrote:
>>> From: "Kory Maincent (TI.com)" <kory.maincent at bootlin.com>
>>>
>>> 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)
>>> <kory.maincent at bootlin.com> ---
>>>    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