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

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

[Sean Anderson]

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?

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

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

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

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.

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.

--Sean

Re: [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

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

[Sean Anderson]

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.

--Sean

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

[Anshul Dalal]

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

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

[Sean Anderson]

On 2/20/25 00:22, Anshul Dalal wrote:
> 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.

Sure.

--Sean

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

[Anshul Dalal]

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.

Regards,
Anshul