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

Tue, 20 Oct 2020 04:48:59 -0700 

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-kri...@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

Reply via email to