[PATCH v2 3/3] common/spl: improve error handling in spl_fit

[Mikhail Kshevetskiy]

On 07.06.2025 11:33, Jonas Karlman wrote:
> Hi Mikhail,
>
> On 2025-06-07 00:31, Mikhail Kshevetskiy wrote:
>> 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 <mikhail.kshevetskiy at iopsys.eu>
>> ---
>>  common/spl/spl_fit.c | 18 +++++++++++++++++-
>>  1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c
>> index 783bb84bdb5..438d1ecf124 100644
>> --- a/common/spl/spl_fit.c
>> +++ b/common/spl/spl_fit.c
>> @@ -703,13 +703,29 @@ 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)
> Are we sure 0x0 is not a valid load address on all platforms? On what
> platform are you seeing this being an issue on?
>
> On e.g. Rockchip 0x0 is typically a valid DRAM address and could
> theoretically be used as the load address in xPL, it is highly unlikely
> because SPL is typically loaded to start of DRAM at 0x0.

Actually I think it's hard to find a platform where 0x0 will be a valid
load address. I faced with an issue on airoha arm platform. It have 2 boot
banks (active and inactive) placed on spi-nand or eMMC. I occasionally
flash wrong fit image (with data placed inside fit structure). During boot
almost all available memory was spent to load this wrong image, so it was
not enough memory for loading from inactive image. The board was bricked :-(

>> +		return -EIO;
>>  
>>  	count = info->read(info, offset, size, buf);
>> +	if (!count) {
>> +		/*
>> +		 * The memory allocated by board_spl_fit_buffer_addr()
>> +		 * should be freed. Unfortunately, we don't know what
>> +		 * memory allocation mechanism was used, so we'll hope
>> +		 * for the best and leave it as is.
>> +		 *
>> +		 * 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
>> +		 */
> For platforms using SPL_SYS_MALLOC_SIMPLE this comment does not fully
> make sense. With SPL_SYS_MALLOC_SIMPLE no allocated memory is re-used.
I know this. From my point of view some comment is required here, so peoples
will not spend a lot of time to understand why errors are not handled
here and
why the memory just can't be freed. I know that my comment is not the
best one :-(
> To my knowledge xPL is typically running with size constrains and memory
> leaking and re-use of freed memory is typically not really an issue as
> main purpose of xPL is to load and jump to next phase.
This is true for most cases.
> On what platform have you seen the need for this memory to be freed?
See above.
> Regards,
> Jonas
>
>> +		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)