[PATCH 0/4] Some reserved memory hardenings

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/4] Some reserved memory hardenings

Tero Kristo
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
Reply | Threaded
Open this post in threaded view
|

[PATCH 1/4] lmb/bdinfo: dump lmb info via bdinfo

Tero Kristo
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 <[hidden email]>
---
 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,
--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Reply | Threaded
Open this post in threaded view
|

[PATCH 2/4] image: fdt: bail out with error if no boot time FDT image found

Tero Kristo
In reply to this post by Tero Kristo
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 <[hidden email]>
---
 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);
--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Reply | Threaded
Open this post in threaded view
|

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

Tero Kristo
In reply to this post by Tero Kristo
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 <[hidden email]>
---
 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,
--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Reply | Threaded
Open this post in threaded view
|

[PATCH 4/4] cmd: booti: convert the debug print about image move to printf

Tero Kristo
In reply to this post by Tero Kristo
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 <[hidden email]>
---
 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);
  }
 
--
2.17.1

--
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] lmb/bdinfo: dump lmb info via bdinfo

Tom Rini-4
In reply to this post by Tero Kristo
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 <[hidden email]>
> ---
>  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.

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 1/4] lmb/bdinfo: dump lmb info via bdinfo

Tero Kristo
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 <[hidden email]>
>> ---
>>   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
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 2/4] image: fdt: bail out with error if no boot time FDT image found

Tom Rini-4
In reply to this post by Tero Kristo
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 <[hidden email]>

Applied to u-boot/master, thanks!

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Tom Rini-4
In reply to this post by Tero Kristo
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 <[hidden email]>

Applied to u-boot/master, thanks!

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 4/4] cmd: booti: convert the debug print about image move to printf

Tom Rini-4
In reply to this post by Tero Kristo
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 <[hidden email]>

Applied to u-boot/master, thanks!

--
Tom

signature.asc (673 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Jaehoon Chung
In reply to this post by Tero Kristo
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 <[hidden email]>
> ---
>  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,
>

Reply | Threaded
Open this post in threaded view
|

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

Tero Kristo
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 <[hidden email]>
>> ---
>>   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 | Threaded
Open this post in threaded view
|

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

Jaehoon Chung
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 <[hidden email]>
>>> ---
>>>   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
>