[PATCH 3/4] common: bootm: add checks to verify if ramdisk / fdtimage overlaps OS image

Tero Kristo t-kristo at ti.com
Tue Oct 20 13:47:51 CEST 2020


On 20/10/2020 14:07, Jaehoon Chung wrote:
> Dear Tero,
> 
> On 6/12/20 9:41 PM, Tero Kristo wrote:
>> These cases are typically fatal and are difficult to debug for random
>> users. Add checks for detecting overlapping images and abort if overlap
>> is detected.
> 
> I have a question about your patch.. because I have confused...
> So i want to clear what i missed.
> 
> 
> During booting with ramdisk, my target is stopped.
> 
> ramdisk start address = 0x2700000
> size = 0x800000
> kernel start - 0x2F00000
> 
> bootm 0x2f00000 0x2700000:0x800000 0x02600000
> 
> 
> ramdisk.end is ramdisk_start + size (0x2F00000)
> 
> Then it will be used until 0x2efffff, not 0x2f00000.
> When i have checked, it doesn't overwrite RD image.
> 
>> +	/* check if ramdisk overlaps OS image */
>> +	if (images.rd_start && (((ulong)images.rd_start >= start &&
>> +				 (ulong)images.rd_start <= start + size) ||
>> +				((ulong)images.rd_end >= start &&
>> +				 (ulong)images.rd_end <= start + size))) {
>> +		printf("ERROR: RD image overlaps OS image (OS=0x%lx..0x%lx)\n",
>> +		       start, start + size);> +		return 1;
>> +	}
> 
> Because of hit this condition...So doesn't boot...
> 
> I think that it needs to change the below conditions..
> 
> images.rd_start < start + size or images.rd_start < start + size - 1.
> images.rd_end > start or image.rd_end - 1 >= start

I believe you are actually right. However, both checks for rd_end should 
take the last byte into account, and would need to change the last check 
to be images.rd_end < start + size in addition to your changes above. 
Also, we would need to add a check to see if rd_start < start && rd_end 
 >= start + size in addition to everything we check here to cover every 
possible overlap scenario...

-Tero

> 
> If i misunderstood something, let me know, plz.
> 
> Best Regards,
> Jaehoon Chung
> 
>>
>> Signed-off-by: Tero Kristo <t-kristo at ti.com>
>> ---
>>   cmd/booti.c       |  2 +-
>>   cmd/bootz.c       |  2 +-
>>   common/bootm.c    | 29 +++++++++++++++++++++++++++--
>>   common/bootm_os.c |  4 ++--
>>   include/bootm.h   |  3 ++-
>>   5 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/cmd/booti.c b/cmd/booti.c
>> index ae37975494..6153b229cf 100644
>> --- a/cmd/booti.c
>> +++ b/cmd/booti.c
>> @@ -93,7 +93,7 @@ static int booti_start(struct cmd_tbl *cmdtp, int flag, int argc,
>>   	 * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not
>>   	 * have a header that provide this informaiton.
>>   	 */
>> -	if (bootm_find_images(flag, argc, argv))
>> +	if (bootm_find_images(flag, argc, argv, relocated_addr, image_size))
>>   		return 1;
>>   
>>   	return 0;
>> diff --git a/cmd/bootz.c b/cmd/bootz.c
>> index bc15fd8ec4..1c8b0cf89f 100644
>> --- a/cmd/bootz.c
>> +++ b/cmd/bootz.c
>> @@ -54,7 +54,7 @@ static int bootz_start(struct cmd_tbl *cmdtp, int flag, int argc,
>>   	 * Handle the BOOTM_STATE_FINDOTHER state ourselves as we do not
>>   	 * have a header that provide this informaiton.
>>   	 */
>> -	if (bootm_find_images(flag, argc, argv))
>> +	if (bootm_find_images(flag, argc, argv, zi_start, zi_end - zi_start))
>>   		return 1;
>>   
>>   	return 0;
>> diff --git a/common/bootm.c b/common/bootm.c
>> index 2ed7521520..247b600d9c 100644
>> --- a/common/bootm.c
>> +++ b/common/bootm.c
>> @@ -228,6 +228,8 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc,
>>    * @flag: Ignored Argument
>>    * @argc: command argument count
>>    * @argv: command argument list
>> + * @start: OS image start address
>> + * @size: OS image size
>>    *
>>    * boot_find_images() will attempt to load an available ramdisk,
>>    * flattened device tree, as well as specifically marked
>> @@ -239,7 +241,8 @@ static int bootm_find_os(struct cmd_tbl *cmdtp, int flag, int argc,
>>    *     0, if all existing images were loaded correctly
>>    *     1, if an image is found but corrupted, or invalid
>>    */
>> -int bootm_find_images(int flag, int argc, char *const argv[])
>> +int bootm_find_images(int flag, int argc, char *const argv[], ulong start,
>> +		      ulong size)
>>   {
>>   	int ret;
>>   
>> @@ -251,6 +254,16 @@ int bootm_find_images(int flag, int argc, char *const argv[])
>>   		return 1;
>>   	}
>>   
>> +	/* check if ramdisk overlaps OS image */
>> +	if (images.rd_start && (((ulong)images.rd_start >= start &&
>> +				 (ulong)images.rd_start <= start + size) ||
>> +				((ulong)images.rd_end >= start &&
>> +				 (ulong)images.rd_end <= start + size))) {
>> +		printf("ERROR: RD image overlaps OS image (OS=0x%lx..0x%lx)\n",
>> +		       start, start + size);
>> +		return 1;
>> +	}
>> +
>>   #if IMAGE_ENABLE_OF_LIBFDT
>>   	/* find flattened device tree */
>>   	ret = boot_get_fdt(flag, argc, argv, IH_ARCH_DEFAULT, &images,
>> @@ -259,6 +272,18 @@ int bootm_find_images(int flag, int argc, char *const argv[])
>>   		puts("Could not find a valid device tree\n");
>>   		return 1;
>>   	}
>> +
>> +	/* check if FDT overlaps OS image */
>> +	if (images.ft_addr &&
>> +	    (((ulong)images.ft_addr >= start &&
>> +	      (ulong)images.ft_addr <= start + size) ||
>> +	     ((ulong)images.ft_addr + images.ft_len >= start &&
>> +	      (ulong)images.ft_addr + images.ft_len <= start + size))) {
>> +		printf("ERROR: FDT image overlaps OS image (OS=0x%lx..0x%lx)\n",
>> +		       start, start + size);
>> +		return 1;
>> +	}
>> +
>>   	if (CONFIG_IS_ENABLED(CMD_FDT))
>>   		set_working_fdt_addr(map_to_sysmem(images.ft_addr));
>>   #endif
>> @@ -294,7 +319,7 @@ static int bootm_find_other(struct cmd_tbl *cmdtp, int flag, int argc,
>>   	     (images.os.type == IH_TYPE_MULTI)) &&
>>   	    (images.os.os == IH_OS_LINUX ||
>>   		 images.os.os == IH_OS_VXWORKS))
>> -		return bootm_find_images(flag, argc, argv);
>> +		return bootm_find_images(flag, argc, argv, 0, 0);
>>   
>>   	return 0;
>>   }
>> diff --git a/common/bootm_os.c b/common/bootm_os.c
>> index 55296483f7..a3c360e80a 100644
>> --- a/common/bootm_os.c
>> +++ b/common/bootm_os.c
>> @@ -495,7 +495,7 @@ static int do_bootm_tee(int flag, int argc, char *const argv[],
>>   		return ret;
>>   
>>   	/* Locate FDT etc */
>> -	ret = bootm_find_images(flag, argc, argv);
>> +	ret = bootm_find_images(flag, argc, argv, 0, 0);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -516,7 +516,7 @@ static int do_bootm_efi(int flag, int argc, char *const argv[],
>>   		return 0;
>>   
>>   	/* Locate FDT, if provided */
>> -	ret = bootm_find_images(flag, argc, argv);
>> +	ret = bootm_find_images(flag, argc, argv, 0, 0);
>>   	if (ret)
>>   		return ret;
>>   
>> diff --git a/include/bootm.h b/include/bootm.h
>> index 1e7f29e134..0350c349f3 100644
>> --- a/include/bootm.h
>> +++ b/include/bootm.h
>> @@ -53,7 +53,8 @@ int boot_selected_os(int argc, char *const argv[], int state,
>>   ulong bootm_disable_interrupts(void);
>>   
>>   /* This is a special function used by booti/bootz */
>> -int bootm_find_images(int flag, int argc, char *const argv[]);
>> +int bootm_find_images(int flag, int argc, char *const argv[], ulong start,
>> +		      ulong size);
>>   
>>   int do_bootm_states(struct cmd_tbl *cmdtp, int flag, int argc,
>>   		    char *const argv[], int states, bootm_headers_t *images,
>>
> 

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki


More information about the U-Boot mailing list