[BUG report] spl: image size check fails in spl_load()

[Anshul Dalal]

On Sat Feb 15, 2025 at 11:18 PM IST, Sean Anderson wrote:
> On 2/14/25 06:12, Anshul Dalal wrote:
> > Hi all!
> > 
> > I was trying to implement falcon boot on TI AM62x EVM with the kernel image on
> > SD card's filesystem but the following check in `_spl_load` at
> > `include/spl_load.h:95` fails to -EIO as per the latest commit [89d3333]:
> > 
> > 	return read < spl_image->size ? -EIO : 0;
> > 
> > The check seems to be comparing the image size gathered from the header
> > (spl_image->size) with the number of bytes read form the loader.
> > 
> >  From spl_load.h:
> > 
> > 	ret = spl_parse_image_header(spl_image, bootdev, header);
> > 	if (ret)
> > 		return ret;
> > 
> > 	base_offset = spl_image->offset;
> > 	/* Only NOR sets this flag. */
> > 	if (IS_ENABLED(CONFIG_SPL_NOR_SUPPORT) &&
> > 	    spl_image->flags & SPL_COPY_PAYLOAD_ONLY)
> > 		base_offset += sizeof(*header);
> > 	image_offset = ALIGN_DOWN(base_offset, spl_get_bl_len(info));
> > 	overhead = base_offset - image_offset;
> > 	size = ALIGN(spl_image->size + overhead, spl_get_bl_len(info));
> > 
> > 	read = info->read(info, offset + image_offset, size,
> > 			  map_sysmem(spl_image->load_addr - overhead, size));
> > 
> > 	if (read < 0)
> > 		return read;
> > 
> > 	return read < spl_image->size ? -EIO : 0;
> > 
> > During kernel build process the header size is computed including the BSS
> > whereas it's removed when creating the uncompressed image. Therefore the size
> > of the uncompressed image on filesystem will be smaller than the size specified
> > in the header. Which leads to failure of the above check.
> > 
> >  From linux kernel's `arch/arm64/kernel/image.h:63`:
> > 
> > 	#define HEAD_SYMBOLS						\
> > 		DEFINE_IMAGE_LE64(_kernel_size_le, _end - _text);	\
> > 		DEFINE_IMAGE_LE64(_kernel_flags_le, __HEAD_FLAGS);
> > 
> > Disabling the check leads to a successful boot directly to the kernel.
> > Therefore it seems like the check is non functional as the size in the kernel
> > header does not correspond with the file size of the kernel image.
>
> Did this work before v2024.04?
>

No, the check existed by v2024.04. I tested on the commit prior to
775074165d97 (the commit that added the check) and falcon boot works.

> How exactly are you loading your image? E.g. what are the values of
>

I have my DTB and kernel Image on the 1st partition of the SD card which
is FAT. Which also includes the u-boot.img in case falcon fails and we
need a fallback.

> CONFIG_SPL_OS_BOOT
> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR
> CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION
> CONFIG_SPL_FALCON_BOOT_MMCSD
> CONFIG_SPL_FS_FAT
> CONFIG_SPL_FS_EXT4
> CONFIG_SPL_FS_LOAD_PAYLOAD_NAME
> CONFIG_SUPPORT_EMMC_BOOT
>

Since I'm not booting from RAW mmc but instead FS boot, I don't think
the SYS_MMCSD_RAW_* configs are relevant but in any case:

CONFIG_SPL_OS_BOOT=y
CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
# CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_PARTITION is not set
# CONFIG_SPL_FALCON_BOOT_MMCSD is not set
CONFIG_SPL_FS_FAT=y
# CONFIG_SPL_FS_EXT4 is not set
CONFIG_SPL_FS_LOAD_PAYLOAD_NAME="u-boot.img"
# CONFIG_SUPPORT_EMMC_BOOT is not set

Some other relevant configs:

CONFIG_SYS_MMCSD_FS_BOOT=y
CONFIG_SYS_MMCSD_FS_BOOT_PARTITION=1
CONFIG_SPL_PAYLOAD_ARGS_ADDR=0x82000000
CONFIG_SPL_FS_LOAD_KERNEL_NAME="Image"
CONFIG_SPL_FS_LOAD_ARGS_NAME="k3-am625-sk.dtb"

>  From what I can tell, the OS_BOOT path should not call spl_load in the first place.

It is called from spl_load_image_fat which is in turn called by
spl_load_image_fat_os as it's last return statement during falcon boot.

>
> In any case, the root problem is that the size reported by the kernel is actually the
> space the kernel will need when it is loaded, and not the size of the data to load
> (which we need). So if we have a short read, we have no way of knowing if the filesystem
> is corrupt, the image was truncated while writing, or if it's just missing the bss. And
> we still have to rely of the image size so that we can load from e.g. NAND or SPI where
> there is no filesystem.
>

There seems to be no way to get the actual size of the kernel image just
from the image header. I think we should drop the check (at least in
case of FS boot without a FIT image). On NAND or SPI, the size from the
header would be larger than the actual file size. So, at worst we would
have read some extra data where BSS was supposed to be anyways.

> One way to fix this could be to move the length check to spl_load_info->read. This would
> involve updating all the callers and callees.
>

I think it would be better handled in spl_load where we can more easily
have separate checks for FIT and a kernel image whereas
spl_info_info->read would have no way of knowing if the binary type
without passing in the header as well.

I suggest we do something like the following in spl_load:

	if (image_get_magic(header) == FDT_MAGIC)
		return read < spl_image->size ? -EIO : 0;
	else
		return read == 0 ? -EIO : 0;

- Anshul