[PATCH 0/4] Some reserved memory hardenings
Hi,
This series applies some reserved memory checks to the boot commands, and enhances the bdinfo command to dump out reserved memory sections. Motivation behind these is mostly to help detecting overwriting either DTB, ramdisk or Linux image with other images. Right now, the boot in most cases just continues but will hang before anything is printed out from Linux. In some cases the boot continues even if u-boot detects something that is terribly wrong (like DTB has been overwritten.)
Patch #3 has some holes in it (it does not pass the OS image / size in some cases.) I wasn't quite sure what parameter to pass in these cases so I just left them at zero, meaning no checks are done.
The layout of the reserved memory section printout with patch #1 could maybe also made look bit neater, I just re-used the existing lmb debug functionality for this purpose.
-Tero
-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Dump lmb status from the bdinfo command. This is useful for seeing the reserved memory regions from the u-boot cmdline.
Signed-off-by: Tero Kristo t-kristo@ti.com --- cmd/bdinfo.c | 8 +++++++- include/lmb.h | 1 + lib/lmb.c | 42 +++++++++++++++++++++++------------------- 3 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c index dba552b03f..05e1b27de0 100644 --- a/cmd/bdinfo.c +++ b/cmd/bdinfo.c @@ -10,6 +10,7 @@ #include <common.h> #include <command.h> #include <env.h> +#include <lmb.h> #include <net.h> #include <vsprintf.h> #include <asm/cache.h> @@ -365,8 +366,13 @@ static int do_bdinfo(struct cmd_tbl *cmdtp, int flag, int argc, #if CONFIG_IS_ENABLED(MULTI_DTB_FIT) print_num("multi_dtb_fit", (ulong)gd->multi_dtb_fit); #endif - if (gd->fdt_blob) + if (gd->fdt_blob) { + struct lmb lmb; print_num("fdt_blob", (ulong)gd->fdt_blob); + lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob); + lmb_dump_all_force(&lmb); + } + print_cpu_word_size();
return 0; diff --git a/include/lmb.h b/include/lmb.h index 3b338dfee0..0d90ab1d65 100644 --- a/include/lmb.h +++ b/include/lmb.h @@ -48,6 +48,7 @@ extern int lmb_is_reserved(struct lmb *lmb, phys_addr_t addr); extern long lmb_free(struct lmb *lmb, phys_addr_t base, phys_size_t size);
extern void lmb_dump_all(struct lmb *lmb); +extern void lmb_dump_all_force(struct lmb *lmb);
static inline phys_size_t lmb_size_bytes(struct lmb_region *type, unsigned long region_nr) diff --git a/lib/lmb.c b/lib/lmb.c index 008bcc7930..d62f25f9de 100644 --- a/lib/lmb.c +++ b/lib/lmb.c @@ -14,33 +14,37 @@
#define LMB_ALLOC_ANYWHERE 0
-void lmb_dump_all(struct lmb *lmb) +void lmb_dump_all_force(struct lmb *lmb) { -#ifdef DEBUG unsigned long i;
- debug("lmb_dump_all:\n"); - debug(" memory.cnt = 0x%lx\n", lmb->memory.cnt); - debug(" memory.size = 0x%llx\n", - (unsigned long long)lmb->memory.size); + printf("lmb_dump_all:\n"); + printf(" memory.cnt = 0x%lx\n", lmb->memory.cnt); + printf(" memory.size = 0x%llx\n", + (unsigned long long)lmb->memory.size); for (i = 0; i < lmb->memory.cnt; i++) { - debug(" memory.reg[0x%lx].base = 0x%llx\n", i, - (unsigned long long)lmb->memory.region[i].base); - debug(" .size = 0x%llx\n", - (unsigned long long)lmb->memory.region[i].size); + printf(" memory.reg[0x%lx].base = 0x%llx\n", i, + (unsigned long long)lmb->memory.region[i].base); + printf(" .size = 0x%llx\n", + (unsigned long long)lmb->memory.region[i].size); }
- debug("\n reserved.cnt = 0x%lx\n", - lmb->reserved.cnt); - debug(" reserved.size = 0x%llx\n", - (unsigned long long)lmb->reserved.size); + printf("\n reserved.cnt = 0x%lx\n", lmb->reserved.cnt); + printf(" reserved.size = 0x%llx\n", + (unsigned long long)lmb->reserved.size); for (i = 0; i < lmb->reserved.cnt; i++) { - debug(" reserved.reg[0x%lx].base = 0x%llx\n", i, - (unsigned long long)lmb->reserved.region[i].base); - debug(" .size = 0x%llx\n", - (unsigned long long)lmb->reserved.region[i].size); + printf(" reserved.reg[0x%lx].base = 0x%llx\n", i, + (unsigned long long)lmb->reserved.region[i].base); + printf(" .size = 0x%llx\n", + (unsigned long long)lmb->reserved.region[i].size); } -#endif /* DEBUG */ +} + +void lmb_dump_all(struct lmb *lmb) +{ +#ifdef DEBUG + lmb_dump_all_force(lmb); +#endif }
static long lmb_addrs_overlap(phys_addr_t base1, phys_size_t size1,
On Fri, Jun 12, 2020 at 03:41:18PM +0300, Tero Kristo wrote:
Dump lmb status from the bdinfo command. This is useful for seeing the reserved memory regions from the u-boot cmdline.
Signed-off-by: Tero Kristo t-kristo@ti.com
cmd/bdinfo.c | 8 +++++++- include/lmb.h | 1 + lib/lmb.c | 42 +++++++++++++++++++++++------------------- 3 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c index dba552b03f..05e1b27de0 100644 --- a/cmd/bdinfo.c +++ b/cmd/bdinfo.c @@ -10,6 +10,7 @@ #include <common.h> #include <command.h> #include <env.h> +#include <lmb.h> #include <net.h> #include <vsprintf.h> #include <asm/cache.h> @@ -365,8 +366,13 @@ static int do_bdinfo(struct cmd_tbl *cmdtp, int flag, int argc, #if CONFIG_IS_ENABLED(MULTI_DTB_FIT) print_num("multi_dtb_fit", (ulong)gd->multi_dtb_fit); #endif
- if (gd->fdt_blob)
if (gd->fdt_blob) {
struct lmb lmb;print_num("fdt_blob", (ulong)gd->fdt_blob);
lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);lmb_dump_all_force(&lmb);}
print_cpu_word_size();
return 0;
Can you please rebase this on top of master? Simon's series to cleanup bdinfo stuff has been applied, thanks. The rest of your changes are fine and under test now.
On 17/07/2020 16:29, Tom Rini wrote:
On Fri, Jun 12, 2020 at 03:41:18PM +0300, Tero Kristo wrote:
Dump lmb status from the bdinfo command. This is useful for seeing the reserved memory regions from the u-boot cmdline.
Signed-off-by: Tero Kristo t-kristo@ti.com
cmd/bdinfo.c | 8 +++++++- include/lmb.h | 1 + lib/lmb.c | 42 +++++++++++++++++++++++------------------- 3 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/cmd/bdinfo.c b/cmd/bdinfo.c index dba552b03f..05e1b27de0 100644 --- a/cmd/bdinfo.c +++ b/cmd/bdinfo.c @@ -10,6 +10,7 @@ #include <common.h> #include <command.h> #include <env.h> +#include <lmb.h> #include <net.h> #include <vsprintf.h> #include <asm/cache.h> @@ -365,8 +366,13 @@ static int do_bdinfo(struct cmd_tbl *cmdtp, int flag, int argc, #if CONFIG_IS_ENABLED(MULTI_DTB_FIT) print_num("multi_dtb_fit", (ulong)gd->multi_dtb_fit); #endif
- if (gd->fdt_blob)
if (gd->fdt_blob) {
struct lmb lmb;print_num("fdt_blob", (ulong)gd->fdt_blob);
lmb_init_and_reserve(&lmb, gd->bd, (void *)gd->fdt_blob);lmb_dump_all_force(&lmb);}
print_cpu_word_size();
return 0;
Can you please rebase this on top of master? Simon's series to cleanup bdinfo stuff has been applied, thanks. The rest of your changes are fine and under test now.
Yeah, I can repost this single patch. Thanks for the heads-up.
-Tero
-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Currently the boot continues if the FDT image is clearly corrupted, which just causes the loaded OS to hang. Abort boot properly if the FDT is corrupted.
Signed-off-by: Tero Kristo t-kristo@ti.com --- common/image-fdt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/common/image-fdt.c b/common/image-fdt.c index b63e772bd6..7005b34966 100644 --- a/common/image-fdt.c +++ b/common/image-fdt.c @@ -426,7 +426,7 @@ int boot_get_fdt(int flag, int argc, char *const argv[], uint8_t arch, break; default: puts("ERROR: Did not find a cmdline Flattened Device Tree\n"); - goto no_fdt; + goto error; }
printf(" Booting using the fdt blob at %#08lx\n", fdt_addr);
On Fri, Jun 12, 2020 at 03:41:19PM +0300, Tero Kristo wrote:
Currently the boot continues if the FDT image is clearly corrupted, which just causes the loaded OS to hang. Abort boot properly if the FDT is corrupted.
Signed-off-by: Tero Kristo t-kristo@ti.com
Applied to u-boot/master, thanks!
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.
Signed-off-by: Tero Kristo t-kristo@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,
On Fri, Jun 12, 2020 at 03:41:20PM +0300, 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.
Signed-off-by: Tero Kristo t-kristo@ti.com
Applied to u-boot/master, thanks!
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
If i misunderstood something, let me know, plz.
Best Regards, Jaehoon Chung
Signed-off-by: Tero Kristo t-kristo@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 correctly1, 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,
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@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 correctly1, 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)@@ -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
On 10/20/20 8:47 PM, Tero Kristo wrote:
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...
diff --git a/common/bootm.c b/common/bootm.c index b3377490b3..36273be1cf 100644 --- a/common/bootm.c +++ b/common/bootm.c @@ -256,9 +256,11 @@ int bootm_find_images(int flag, int argc, char *const argv[], ulong start,
/* 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))) { + (ulong)images.rd_start < start + size) || + ((ulong)images.rd_end > start && + (ulong)images.rd_end <= start + size)) || + ((ulong)images.rd_start < 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 you're ok, then i will send patch for fixing.
Best Regards, Jaehoon Chung
-Tero
If i misunderstood something, let me know, plz.
Best Regards, Jaehoon Chung
Signed-off-by: Tero Kristo t-kristo@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
Moving of the OS image may have some nasty side effects like corrupting DTB. Convert the current debug print to printf so that the relocation of the OS is always obvious to the user.
Signed-off-by: Tero Kristo t-kristo@ti.com --- cmd/booti.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/cmd/booti.c b/cmd/booti.c index 6153b229cf..ad04ebd8c3 100644 --- a/cmd/booti.c +++ b/cmd/booti.c @@ -79,7 +79,8 @@ static int booti_start(struct cmd_tbl *cmdtp, int flag, int argc,
/* Handle BOOTM_STATE_LOADOS */ if (relocated_addr != ld) { - debug("Moving Image from 0x%lx to 0x%lx\n", ld, relocated_addr); + printf("Moving Image from 0x%lx to 0x%lx, end=%lx\n", ld, + relocated_addr, relocated_addr + image_size); memmove((void *)relocated_addr, (void *)ld, image_size); }
On Fri, Jun 12, 2020 at 03:41:21PM +0300, Tero Kristo wrote:
Moving of the OS image may have some nasty side effects like corrupting DTB. Convert the current debug print to printf so that the relocation of the OS is always obvious to the user.
Signed-off-by: Tero Kristo t-kristo@ti.com
Applied to u-boot/master, thanks!
participants (3)
-
Jaehoon Chung -
Tero Kristo -
Tom Rini