Hi Heiko,

Did you see my last email? The one that bounced with a mime header and where I attached a patch file.

I just wonder if its not better to switch the define to be

if (CONFIG_SIGNATURE_VERIFICATION_WITH_LEGACY_SIDE_DOOR). It can become mutually exclusive with the existing signature verification define.

That way the legacy stuff is removed automatically upon requesting verification unless defined otherwise. When you fail to boot an unsigned legacy kernel then its kind of obvious that you have to solve something but if you implement verified boot and forget this new variable then you leave a security hole.

In my last email I also discussed my confusion regard the 'required' variable. Similar argument to the above plus some other thoughts.

Thanks,

Mike.

On 05/08/2014 01:05 PM, Heiko Schocher wrote:
Disabling legacy image format is useful when using
signed FIT images with required signature check.
Use CONFIG_DISABLE_IMAGE_FORMAT_LEGACY in the board
config file.

Signed-off-by: Heiko Schocher <h...@denx.de>
Cc: Simon Glass <s...@chromium.org>
Cc: Lars Steubesand <lars.steubes...@philips.com>
Cc: Mike Pearce <m...@kaew.be>
Cc: Wolfgang Denk <w...@denx.de>
---
  README                       |  5 +++++
  common/cmd_bootm.c           | 14 ++++++++++++++
  common/cmd_disk.c            |  4 ++++
  common/cmd_fdc.c             |  4 ++++
  common/cmd_fpga.c            |  2 ++
  common/cmd_nand.c            |  4 ++++
  common/cmd_source.c          |  4 ++++
  common/cmd_ximg.c            |  7 ++++++-
  common/image-fdt.c           | 10 ++++++++--
  common/image.c               | 23 ++++++++++++++++-------
  doc/uImage.FIT/signature.txt |  3 +++
  include/image.h              |  2 ++
  12 files changed, 72 insertions(+), 10 deletions(-)

diff --git a/README b/README
index b973344..b1f0ea3 100644
--- a/README
+++ b/README
@@ -3152,6 +3152,11 @@ FIT uImage format:
                using a hash signed and verified using RSA. See
                doc/uImage.FIT/signature.txt for more details.
+ CONFIG_DISABLE_IMAGE_FORMAT_LEGACY
+               WARNING: When relying on signed FIT images with required
+               signature check the legacy image format should be disabled by
+               using this define.
+
  - Standalone program support:
                CONFIG_STANDALONE_LOAD_ADDR
diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
index c243a5b..69b86ee 100644
--- a/common/cmd_bootm.c
+++ b/common/cmd_bootm.c
@@ -233,6 +233,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int 
argc,
/* get image parameters */
        switch (genimg_get_format(os_hdr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                images.os.type = image_get_type(os_hdr);
                images.os.comp = image_get_comp(os_hdr);
@@ -241,6 +242,7 @@ static int bootm_find_os(cmd_tbl_t *cmdtp, int flag, int 
argc,
                images.os.end = image_get_image_end(os_hdr);
                images.os.load = image_get_load(os_hdr);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                if (fit_image_get_type(images.fit_hdr_os,
@@ -838,6 +840,7 @@ int bootm_maybe_autostart(cmd_tbl_t *cmdtp, const char *cmd)
        return 0;
  }
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
  /**
   * image_get_kernel - verify legacy format kernel image
   * @img_addr: in RAM address of the legacy format image to be verified
@@ -888,6 +891,7 @@ static image_header_t *image_get_kernel(ulong img_addr, int 
verify)
        }
        return hdr;
  }
+#endif
/**
   * boot_get_kernel - find kernel image
@@ -905,7 +909,9 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int 
flag, int argc,
                char * const argv[], bootm_headers_t *images, ulong *os_data,
                ulong *os_len)
  {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        image_header_t  *hdr;
+#endif
        ulong           img_addr;
        const void *buf;
  #if defined(CONFIG_FIT)
@@ -943,6 +949,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int 
flag, int argc,
        *os_data = *os_len = 0;
        buf = map_sysmem(img_addr, 0);
        switch (genimg_get_format(buf)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                printf("## Booting kernel from Legacy Image at %08lx ...\n",
                                img_addr);
@@ -985,6 +992,7 @@ static const void *boot_get_kernel(cmd_tbl_t *cmdtp, int 
flag, int argc,
                images->legacy_hdr_valid = 1;
                bootstage_mark(BOOTSTAGE_ID_DECOMP_IMAGE);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                os_noffset = fit_image_load(images, FIT_KERNEL_PROP,
@@ -1114,6 +1122,7 @@ static int image_info(ulong addr)
        printf("\n## Checking Image at %08lx ...\n", addr);
switch (genimg_get_format(hdr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                puts("   Legacy image found\n");
                if (!image_check_magic(hdr)) {
@@ -1135,6 +1144,7 @@ static int image_info(ulong addr)
                }
                puts("OK\n");
                return 0;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                puts("   FIT image found\n");
@@ -1194,6 +1204,7 @@ static int do_imls_nor(void)
                                goto next_sector;
switch (genimg_get_format(hdr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
                        case IMAGE_FORMAT_LEGACY:
                                if (!image_check_hcrc(hdr))
                                        goto next_sector;
@@ -1208,6 +1219,7 @@ static int do_imls_nor(void)
                                        puts("OK\n");
                                }
                                break;
+#endif
  #if defined(CONFIG_FIT)
                        case IMAGE_FORMAT_FIT:
                                if (!fit_check_format(hdr))
@@ -1342,12 +1354,14 @@ static int do_imls_nand(void)
                        }
switch (genimg_get_format(buffer)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
                        case IMAGE_FORMAT_LEGACY:
                                header = (const image_header_t *)buffer;
len = image_get_image_size(header);
                                nand_imls_legacyimage(nand, nand_dev, off, len);
                                break;
+#endif
  #if defined(CONFIG_FIT)
                        case IMAGE_FORMAT_FIT:
                                len = fit_get_size(buffer);
diff --git a/common/cmd_disk.c b/common/cmd_disk.c
index 3e457f6..9b1ba7f 100644
--- a/common/cmd_disk.c
+++ b/common/cmd_disk.c
@@ -17,7 +17,9 @@ int common_diskboot(cmd_tbl_t *cmdtp, const char *intf, int 
argc,
        ulong addr = CONFIG_SYS_LOAD_ADDR;
        ulong cnt;
        disk_partition_t info;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        image_header_t *hdr;
+#endif
        block_dev_desc_t *dev_desc;
#if defined(CONFIG_FIT)
@@ -62,6 +64,7 @@ int common_diskboot(cmd_tbl_t *cmdtp, const char *intf, int 
argc,
        bootstage_mark(BOOTSTAGE_ID_IDE_PART_READ);
switch (genimg_get_format((void *) addr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                hdr = (image_header_t *) addr;
@@ -78,6 +81,7 @@ int common_diskboot(cmd_tbl_t *cmdtp, const char *intf, int argc, cnt = image_get_image_size(hdr);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                fit_hdr = (const void *) addr;
diff --git a/common/cmd_fdc.c b/common/cmd_fdc.c
index 1cfb656..409a216 100644
--- a/common/cmd_fdc.c
+++ b/common/cmd_fdc.c
@@ -635,7 +635,9 @@ int do_fdcboot (cmd_tbl_t *cmdtp, int flag, int argc, char 
* const argv[])
        FD_GEO_STRUCT *pFG = (FD_GEO_STRUCT *)floppy_type;
        FDC_COMMAND_STRUCT *pCMD = &cmd;
        unsigned long addr,imsize;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        image_header_t *hdr;  /* used for fdc boot */
+#endif
        unsigned char boot_drive;
        int i,nrofblk;
  #if defined(CONFIG_FIT)
@@ -689,12 +691,14 @@ int do_fdcboot (cmd_tbl_t *cmdtp, int flag, int argc, 
char * const argv[])
        }
switch (genimg_get_format ((void *)addr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                hdr = (image_header_t *)addr;
                image_print_contents (hdr);
imsize = image_get_image_size (hdr);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                fit_hdr = (const void *)addr;
diff --git a/common/cmd_fpga.c b/common/cmd_fpga.c
index 010cd24..34eb3f0 100644
--- a/common/cmd_fpga.c
+++ b/common/cmd_fpga.c
@@ -155,6 +155,7 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char 
*const argv[])
case FPGA_LOADMK:
                switch (genimg_get_format(fpga_data)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
                case IMAGE_FORMAT_LEGACY:
                        {
                                image_header_t *hdr =
@@ -182,6 +183,7 @@ int do_fpga(cmd_tbl_t *cmdtp, int flag, int argc, char 
*const argv[])
                                rc = fpga_load(dev, (void *)data, data_size);
                        }
                        break;
+#endif
  #if defined(CONFIG_FIT)
                case IMAGE_FORMAT_FIT:
                        {
diff --git a/common/cmd_nand.c b/common/cmd_nand.c
index 04ab0f1..67d0796 100644
--- a/common/cmd_nand.c
+++ b/common/cmd_nand.c
@@ -904,7 +904,9 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t 
*nand,
        int r;
        char *s;
        size_t cnt;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        image_header_t *hdr;
+#endif
  #if defined(CONFIG_FIT)
        const void *fit_hdr = NULL;
  #endif
@@ -930,6 +932,7 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t 
*nand,
        bootstage_mark(BOOTSTAGE_ID_NAND_HDR_READ);
switch (genimg_get_format ((void *)addr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                hdr = (image_header_t *)addr;
@@ -938,6 +941,7 @@ static int nand_load_image(cmd_tbl_t *cmdtp, nand_info_t *nand, cnt = image_get_image_size (hdr);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                fit_hdr = (const void *)addr;
diff --git a/common/cmd_source.c b/common/cmd_source.c
index 54ffd16..51043a1 100644
--- a/common/cmd_source.c
+++ b/common/cmd_source.c
@@ -29,7 +29,9 @@ int
  source (ulong addr, const char *fit_uname)
  {
        ulong           len;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        const image_header_t *hdr;
+#endif
        ulong           *data;
        int             verify;
        void *buf;
@@ -44,6 +46,7 @@ source (ulong addr, const char *fit_uname)
buf = map_sysmem(addr, 0);
        switch (genimg_get_format(buf)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
                hdr = buf;
@@ -84,6 +87,7 @@ source (ulong addr, const char *fit_uname)
                 */
                while (*data++);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                if (fit_uname == NULL) {
diff --git a/common/cmd_ximg.c b/common/cmd_ximg.c
index 65a8319..527b1bd 100644
--- a/common/cmd_ximg.c
+++ b/common/cmd_ximg.c
@@ -32,10 +32,13 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * 
const argv[])
  {
        ulong           addr = load_addr;
        ulong           dest = 0;
-       ulong           data, len, count;
+       ulong           data, len;
        int             verify;
        int             part = 0;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
+       ulong           count;
        image_header_t  *hdr = NULL;
+#endif
  #if defined(CONFIG_FIT)
        const char      *uname = NULL;
        const void*     fit_hdr;
@@ -64,6 +67,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * 
const argv[])
        }
switch (genimg_get_format((void *)addr)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        case IMAGE_FORMAT_LEGACY:
printf("## Copying part %d from legacy image "
@@ -114,6 +118,7 @@ do_imgextract(cmd_tbl_t * cmdtp, int flag, int argc, char * 
const argv[])
image_multi_getimg(hdr, part, &data, &len);
                break;
+#endif
  #if defined(CONFIG_FIT)
        case IMAGE_FORMAT_FIT:
                if (uname == NULL) {
diff --git a/common/image-fdt.c b/common/image-fdt.c
index a54a919..2a3262d 100644
--- a/common/image-fdt.c
+++ b/common/image-fdt.c
@@ -29,6 +29,7 @@ static void fdt_error(const char *msg)
        puts(" - must RESET the board to recover.\n");
  }
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
  static const image_header_t *image_get_fdt(ulong fdt_addr)
  {
        const image_header_t *fdt_hdr = map_sysmem(fdt_addr, 0);
@@ -61,6 +62,7 @@ static const image_header_t *image_get_fdt(ulong fdt_addr)
        }
        return fdt_hdr;
  }
+#endif
/**
   * boot_fdt_add_mem_rsv_regions - Mark the memreserve sections as unusable
@@ -220,11 +222,13 @@ error:
  int boot_get_fdt(int flag, int argc, char * const argv[], uint8_t arch,
                bootm_headers_t *images, char **of_flat_tree, ulong *of_size)
  {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        const image_header_t *fdt_hdr;
+       ulong           load, load_end;
+       ulong           image_start, image_data, image_end;
+#endif
        ulong           fdt_addr;
        char            *fdt_blob = NULL;
-       ulong           image_start, image_data, image_end;
-       ulong           load, load_end;
        void            *buf;
  #if defined(CONFIG_FIT)
        const char      *fit_uname_config = images->fit_uname_cfg;
@@ -298,6 +302,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], 
uint8_t arch,
                 */
                buf = map_sysmem(fdt_addr, 0);
                switch (genimg_get_format(buf)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
                case IMAGE_FORMAT_LEGACY:
                        /* verify fdt_addr points to a valid image header */
                        printf("## Flattened Device Tree from Legacy Image at 
%08lx\n",
@@ -337,6 +342,7 @@ int boot_get_fdt(int flag, int argc, char * const argv[], 
uint8_t arch,
fdt_addr = load;
                        break;
+#endif
                case IMAGE_FORMAT_FIT:
                        /*
                         * This case will catch both: new uImage format
diff --git a/common/image.c b/common/image.c
index 9c6bec5..ae7105e 100644
--- a/common/image.c
+++ b/common/image.c
@@ -44,8 +44,10 @@ extern int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, 
char * const argv[]);
DECLARE_GLOBAL_DATA_PTR; +#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
  static const image_header_t *image_get_ramdisk(ulong rd_addr, uint8_t arch,
                                                int verify);
+#endif
  #else
  #include "mkimage.h"
  #include <u-boot/md5.h>
@@ -328,6 +330,7 @@ void image_print_contents(const void *ptr)
#ifndef USE_HOSTCC
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
  /**
   * image_get_ramdisk - get and verify ramdisk image
   * @rd_addr: ramdisk image start address
@@ -389,6 +392,7 @@ static const image_header_t *image_get_ramdisk(ulong 
rd_addr, uint8_t arch,
return rd_hdr;
  }
+#endif
  #endif /* !USE_HOSTCC */
/*****************************************************************************/
@@ -652,20 +656,19 @@ int genimg_get_comp_id(const char *name)
   */
  int genimg_get_format(const void *img_addr)
  {
-       ulong format = IMAGE_FORMAT_INVALID;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        const image_header_t *hdr;
hdr = (const image_header_t *)img_addr;
        if (image_check_magic(hdr))
-               format = IMAGE_FORMAT_LEGACY;
+               return IMAGE_FORMAT_LEGACY;
+#endif
  #if defined(CONFIG_FIT) || defined(CONFIG_OF_LIBFDT)
-       else {
-               if (fdt_check_header(img_addr) == 0)
-                       format = IMAGE_FORMAT_FIT;
-       }
+       if (fdt_check_header(img_addr) == 0)
+                       return IMAGE_FORMAT_FIT;
  #endif
- return format;
+       return IMAGE_FORMAT_INVALID;
  }
/**
@@ -707,12 +710,14 @@ ulong genimg_get_image(ulong img_addr)
/* get data size */
                switch (genimg_get_format(buf)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
                case IMAGE_FORMAT_LEGACY:
                        d_size = image_get_data_size(buf);
                        debug("   Legacy format image found at 0x%08lx, "
                                        "size 0x%08lx\n",
                                        ram_addr, d_size);
                        break;
+#endif
  #if defined(CONFIG_FIT)
                case IMAGE_FORMAT_FIT:
                        d_size = fit_get_size(buf) - h_size;
@@ -788,7 +793,9 @@ int boot_get_ramdisk(int argc, char * const argv[], 
bootm_headers_t *images,
  {
        ulong rd_addr, rd_load;
        ulong rd_data, rd_len;
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
        const image_header_t *rd_hdr;
+#endif
        void *buf;
  #ifdef CONFIG_SUPPORT_RAW_INITRD
        char *end;
@@ -871,6 +878,7 @@ int boot_get_ramdisk(int argc, char * const argv[], 
bootm_headers_t *images,
                 */
                buf = map_sysmem(rd_addr, 0);
                switch (genimg_get_format(buf)) {
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
                case IMAGE_FORMAT_LEGACY:
                        printf("## Loading init Ramdisk from Legacy "
                                        "Image at %08lx ...\n", rd_addr);
@@ -886,6 +894,7 @@ int boot_get_ramdisk(int argc, char * const argv[], 
bootm_headers_t *images,
                        rd_len = image_get_data_size(rd_hdr);
                        rd_load = image_get_load(rd_hdr);
                        break;
+#endif
  #if defined(CONFIG_FIT)
                case IMAGE_FORMAT_FIT:
                        rd_noffset = fit_image_load(images, FIT_RAMDISK_PROP,
diff --git a/doc/uImage.FIT/signature.txt b/doc/uImage.FIT/signature.txt
index 9502037..4e32aa0 100644
--- a/doc/uImage.FIT/signature.txt
+++ b/doc/uImage.FIT/signature.txt
@@ -328,6 +328,9 @@ be enabled:
  CONFIG_FIT_SIGNATURE - enable signing and verfication in FITs
  CONFIG_RSA - enable RSA algorithm for signing
+WARNING: When relying on signed FIT images with required signature check
+the legacy image format should be disabled by using
+CONFIG_DISABLE_IMAGE_FORMAT_LEGACY
Testing
  -------
diff --git a/include/image.h b/include/image.h
index 2508d7d..c46b1e0 100644
--- a/include/image.h
+++ b/include/image.h
@@ -410,7 +410,9 @@ enum fit_load_op {
  #ifndef USE_HOSTCC
  /* Image format types, returned by _get_format() routine */
  #define IMAGE_FORMAT_INVALID  0x00
+#if !defined(CONFIG_DISABLE_IMAGE_FORMAT_LEGACY)
  #define IMAGE_FORMAT_LEGACY   0x01    /* legacy image_header based format */
+#endif
  #define IMAGE_FORMAT_FIT      0x02    /* new, libfdt based format */
int genimg_get_format(const void *img_addr);

_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to