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

[Anshul Dalal <anshuld@ti.com>]

On Wed Feb 19, 2025 at 9:17 PM IST, Sean Anderson wrote:
> On 2/18/25 01:07, Anshul Dalal wrote:
> > 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.
>
> Sorry, that's what I meant. E.g. did this work in v2024.01 etc.
>
> Just wanted to determine whether this was a bug or a feature request.
>
> >> 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.
>
> Ah, there it is. This function is used both by OS_BOOT and regular boot. I intentionally
> tried not to touch the OS_BOOT path, but it looks like one change got in anyway.
>
> >>
> >> 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.
>
> Yeah, I thought about this after I sent the email...
>
> > 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;
>
> I would prefer not doing this sort of thing since it grows the size of every SPL, but only a
> few do falcon boot. Maybe it's better to skip the size check only for OS_BOOT and CMD_BOOTI.
>
> Or maybe we can move this hack to spl_fit_read. i.e:
>
> 	ret = fat_read_file(filename, buf, file_offset, size, &actread);
> 	if (ret)
> 		return ret;
>
> 	if (CONFIG_IS_ENABLED(OS_BOOT) && IS_ENABLED(CMD_BOOTI))
> 		return size;
> 	else
> 		return actread;
>
> I think this would have the least impact overall.
>

Looks good to me, we would need to do the same for ext4 in spl_ext.c. I
can send a patch with the fix if you'd like.

- Anshul