From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesper Schmitz Mouridsen Date: Wed, 17 Feb 2021 14:30:56 +0100 Subject: [PATCH 5/8] image: Adjust the workings of fit_check_format() In-Reply-To: <20210216000812.2091481-6-sjg@chromium.org> References: <20210216000812.2091481-1-sjg@chromium.org> <20210216000812.2091481-6-sjg@chromium.org> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Can you avoid the use of ENODATA since it is not defined in FreeBSD's errno.h? Regards Jesper Schmitz Mouridsen On 16.02.2021 01.08, Simon Glass wrote: > At present this function does not accept a size for the FIT. This means > that it must be read from the FIT itself, introducing potential security > risk. Update the function to include a size parameter, which can be > invalid, in which case fit_check_format() calculates it. > > For now no callers pass the size, but this can be updated later. > > Also adjust the return value to an error code so that all the different > types of problems can be distinguished by the user. > > Signed-off-by: Simon Glass > Reported-by: Bruce Monroe > Reported-by: Arie Haenel > Reported-by: Julien Lenoir > --- > > arch/arm/cpu/armv8/sec_firmware.c | 2 +- > cmd/bootefi.c | 2 +- > cmd/bootm.c | 6 ++-- > cmd/disk.c | 2 +- > cmd/fpga.c | 2 +- > cmd/nand.c | 2 +- > cmd/source.c | 2 +- > cmd/ximg.c | 2 +- > common/image-fdt.c | 2 +- > common/image-fit.c | 46 +++++++++++++----------------- > common/splash_source.c | 6 ++-- > common/update.c | 4 +-- > drivers/fpga/socfpga_arria10.c | 6 ++-- > drivers/net/fsl-mc/mc.c | 2 +- > drivers/net/pfe_eth/pfe_firmware.c | 2 +- > include/image.h | 21 +++++++++++++- > tools/fit_common.c | 3 +- > tools/fit_image.c | 2 +- > tools/mkimage.h | 2 ++ > 19 files changed, 66 insertions(+), 50 deletions(-) > > diff --git a/arch/arm/cpu/armv8/sec_firmware.c b/arch/arm/cpu/armv8/sec_firmware.c > index c6c4fcc7e07..267894fbcb3 100644 > --- a/arch/arm/cpu/armv8/sec_firmware.c > +++ b/arch/arm/cpu/armv8/sec_firmware.c > @@ -317,7 +317,7 @@ __weak bool sec_firmware_is_valid(const void *sec_firmware_img) > return false; > } > > - if (!fit_check_format(sec_firmware_img)) { > + if (fit_check_format(sec_firmware_img, IMAGE_SIZE_INVAL)) { > printf("SEC Firmware: Bad firmware image (bad FIT header)\n"); > return false; > } > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index 1583a96be14..271b385edea 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -73,7 +73,7 @@ void efi_set_bootdev(const char *dev, const char *devnr, const char *path, > /* Remember only PE-COFF and FIT images */ > if (efi_check_pe(buffer, buffer_size, NULL) != EFI_SUCCESS) { > #ifdef CONFIG_FIT > - if (!fit_check_format(buffer)) > + if (fit_check_format(buffer, IMAGE_SIZE_INVAL)) > return; > /* > * FIT images of type EFI_OS are started via command bootm. > diff --git a/cmd/bootm.c b/cmd/bootm.c > index 7732b97f635..81c6b939781 100644 > --- a/cmd/bootm.c > +++ b/cmd/bootm.c > @@ -292,7 +292,7 @@ static int image_info(ulong addr) > case IMAGE_FORMAT_FIT: > puts(" FIT image found\n"); > > - if (!fit_check_format(hdr)) { > + if (fit_check_format(hdr, IMAGE_SIZE_INVAL)) { > puts("Bad FIT image format!\n"); > unmap_sysmem(hdr); > return 1; > @@ -369,7 +369,7 @@ static int do_imls_nor(void) > #endif > #if defined(CONFIG_FIT) > case IMAGE_FORMAT_FIT: > - if (!fit_check_format(hdr)) > + if (fit_check_format(hdr, IMAGE_SIZE_INVAL)) > goto next_sector; > > printf("FIT Image at %08lX:\n", (ulong)hdr); > @@ -449,7 +449,7 @@ static int nand_imls_fitimage(struct mtd_info *mtd, int nand_dev, loff_t off, > return ret; > } > > - if (!fit_check_format(imgdata)) { > + if (fit_check_format(imgdata, IMAGE_SIZE_INVAL)) { > free(imgdata); > return 0; > } > diff --git a/cmd/disk.c b/cmd/disk.c > index 0bc3808dfe2..2726115e855 100644 > --- a/cmd/disk.c > +++ b/cmd/disk.c > @@ -114,7 +114,7 @@ int common_diskboot(struct cmd_tbl *cmdtp, const char *intf, int argc, > /* This cannot be done earlier, > * we need complete FIT image in RAM first */ > if (genimg_get_format((void *) addr) == IMAGE_FORMAT_FIT) { > - if (!fit_check_format(fit_hdr)) { > + if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) { > bootstage_error(BOOTSTAGE_ID_IDE_FIT_READ); > puts("** Bad FIT image format\n"); > return 1; > diff --git a/cmd/fpga.c b/cmd/fpga.c > index 8ae1c936fbb..51410a8e424 100644 > --- a/cmd/fpga.c > +++ b/cmd/fpga.c > @@ -330,7 +330,7 @@ static int do_fpga_loadmk(struct cmd_tbl *cmdtp, int flag, int argc, > return CMD_RET_FAILURE; > } > > - if (!fit_check_format(fit_hdr)) { > + if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) { > puts("Bad FIT image format\n"); > return CMD_RET_FAILURE; > } > diff --git a/cmd/nand.c b/cmd/nand.c > index 92d039af8f5..97e117a979a 100644 > --- a/cmd/nand.c > +++ b/cmd/nand.c > @@ -917,7 +917,7 @@ static int nand_load_image(struct cmd_tbl *cmdtp, struct mtd_info *mtd, > #if defined(CONFIG_FIT) > /* This cannot be done earlier, we need complete FIT image in RAM first */ > if (genimg_get_format ((void *)addr) == IMAGE_FORMAT_FIT) { > - if (!fit_check_format (fit_hdr)) { > + if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) { > bootstage_error(BOOTSTAGE_ID_NAND_FIT_READ); > puts ("** Bad FIT image format\n"); > return 1; > diff --git a/cmd/source.c b/cmd/source.c > index b6c709a3d25..71f71528ad2 100644 > --- a/cmd/source.c > +++ b/cmd/source.c > @@ -107,7 +107,7 @@ int image_source_script(ulong addr, const char *fit_uname) > #if defined(CONFIG_FIT) > case IMAGE_FORMAT_FIT: > fit_hdr = buf; > - if (!fit_check_format (fit_hdr)) { > + if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) { > puts ("Bad FIT image format\n"); > return 1; > } > diff --git a/cmd/ximg.c b/cmd/ximg.c > index 159ba516489..ef738ebfa2f 100644 > --- a/cmd/ximg.c > +++ b/cmd/ximg.c > @@ -136,7 +136,7 @@ do_imgextract(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) > "at %08lx ...\n", uname, addr); > > fit_hdr = (const void *)addr; > - if (!fit_check_format(fit_hdr)) { > + if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) { > puts("Bad FIT image format\n"); > return 1; > } > diff --git a/common/image-fdt.c b/common/image-fdt.c > index 0157cce32d5..61ce6e5779f 100644 > --- a/common/image-fdt.c > +++ b/common/image-fdt.c > @@ -400,7 +400,7 @@ int boot_get_fdt(int flag, int argc, char *const argv[], uint8_t arch, > */ > #if CONFIG_IS_ENABLED(FIT) > /* check FDT blob vs FIT blob */ > - if (fit_check_format(buf)) { > + if (!fit_check_format(buf, IMAGE_SIZE_INVAL)) { > ulong load, len; > > fdt_noffset = boot_get_fdt_fit(images, > diff --git a/common/image-fit.c b/common/image-fit.c > index c3dc814115f..f6c0428a96b 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -8,6 +8,8 @@ > * Wolfgang Denk, DENX Software Engineering, wd at denx.de. > */ > > +#define LOG_CATEGORY LOGC_BOOT > + > #ifdef USE_HOSTCC > #include "mkimage.h" > #include > @@ -1566,49 +1568,41 @@ int fit_image_check_comp(const void *fit, int noffset, uint8_t comp) > return (comp == image_comp); > } > > -/** > - * fit_check_format - sanity check FIT image format > - * @fit: pointer to the FIT format image header > - * > - * fit_check_format() runs a basic sanity FIT image verification. > - * Routine checks for mandatory properties, nodes, etc. > - * > - * returns: > - * 1, on success > - * 0, on failure > - */ > -int fit_check_format(const void *fit) > +int fit_check_format(const void *fit, ulong size) > { > + int ret; > + > /* A FIT image must be a valid FDT */ > - if (fdt_check_header(fit)) { > - debug("Wrong FIT format: not a flattened device tree\n"); > - return 0; > + ret = fdt_check_header(fit); > + if (ret) { > + log_debug("Wrong FIT format: not a flattened device tree (err=%d)\n", > + ret); > + return -ENOEXEC; > } > > /* mandatory / node 'description' property */ > - if (fdt_getprop(fit, 0, FIT_DESC_PROP, NULL) == NULL) { > - debug("Wrong FIT format: no description\n"); > - return 0; > + if (!fdt_getprop(fit, 0, FIT_DESC_PROP, NULL)) { > + log_debug("Wrong FIT format: no description\n"); > + return -ENOMSG; > } > > if (IMAGE_ENABLE_TIMESTAMP) { > /* mandatory / node 'timestamp' property */ > - if (fdt_getprop(fit, 0, FIT_TIMESTAMP_PROP, NULL) == NULL) { > - debug("Wrong FIT format: no timestamp\n"); > - return 0; > + if (!fdt_getprop(fit, 0, FIT_TIMESTAMP_PROP, NULL)) { > + log_debug("Wrong FIT format: no timestamp\n"); > + return -ENODATA; > } > } > > /* mandatory subimages parent '/images' node */ > if (fdt_path_offset(fit, FIT_IMAGES_PATH) < 0) { > - debug("Wrong FIT format: no images parent node\n"); > - return 0; > + log_debug("Wrong FIT format: no images parent node\n"); > + return -ENOENT; > } > > - return 1; > + return 0; > } > > - > /** > * fit_conf_find_compat > * @fit: pointer to the FIT format image header > @@ -1945,7 +1939,7 @@ int fit_image_load(bootm_headers_t *images, ulong addr, > printf("## Loading %s from FIT Image at %08lx ...\n", prop_name, addr); > > bootstage_mark(bootstage_id + BOOTSTAGE_SUB_FORMAT); > - if (!fit_check_format(fit)) { > + if (fit_check_format(fit, IMAGE_SIZE_INVAL)) { > printf("Bad FIT %s image format!\n", prop_name); > bootstage_error(bootstage_id + BOOTSTAGE_SUB_FORMAT); > return -ENOEXEC; > diff --git a/common/splash_source.c b/common/splash_source.c > index 2737fc6e7ff..7065200a847 100644 > --- a/common/splash_source.c > +++ b/common/splash_source.c > @@ -337,10 +337,10 @@ static int splash_load_fit(struct splash_location *location, u32 bmp_load_addr) > if (res < 0) > return res; > > - res = fit_check_format(fit_header); > - if (!res) { > + res = fit_check_format(fit_header, IMAGE_SIZE_INVAL); > + if (res) { > debug("Could not find valid FIT image\n"); > - return -EINVAL; > + return ret; > } > > /* Get the splash image node */ > diff --git a/common/update.c b/common/update.c > index a5879cb52c4..f0848954e5d 100644 > --- a/common/update.c > +++ b/common/update.c > @@ -286,7 +286,7 @@ int update_tftp(ulong addr, char *interface, char *devstring) > got_update_file: > fit = map_sysmem(addr, 0); > > - if (!fit_check_format((void *)fit)) { > + if (fit_check_format((void *)fit, IMAGE_SIZE_INVAL)) { > printf("Bad FIT format of the update file, aborting " > "auto-update\n"); > return 1; > @@ -363,7 +363,7 @@ int fit_update(const void *fit) > if (!fit) > return -EINVAL; > > - if (!fit_check_format((void *)fit)) { > + if (fit_check_format((void *)fit, IMAGE_SIZE_INVAL)) { > printf("Bad FIT format of the update file, aborting auto-update\n"); > return -EINVAL; > } > diff --git a/drivers/fpga/socfpga_arria10.c b/drivers/fpga/socfpga_arria10.c > index 4bea7fd900d..b992e6f0805 100644 > --- a/drivers/fpga/socfpga_arria10.c > +++ b/drivers/fpga/socfpga_arria10.c > @@ -566,10 +566,10 @@ static int first_loading_rbf_to_buffer(struct udevice *dev, > if (ret < 0) > return ret; > > - ret = fit_check_format(buffer_p); > - if (!ret) { > + ret = fit_check_format(buffer_p, IMAGE_SIZE_INVAL); > + if (ret) { > debug("FPGA: No valid FIT image was found.\n"); > - return -EBADF; > + return ret; > } > > confs_noffset = fdt_path_offset(buffer_p, FIT_CONFS_PATH); > diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c > index c9cf6a987e1..972db4cf3a0 100644 > --- a/drivers/net/fsl-mc/mc.c > +++ b/drivers/net/fsl-mc/mc.c > @@ -142,7 +142,7 @@ int parse_mc_firmware_fit_image(u64 mc_fw_addr, > return -EINVAL; > } > > - if (!fit_check_format(fit_hdr)) { > + if (fit_check_format(fit_hdr, IMAGE_SIZE_INVAL)) { > printf("fsl-mc: ERR: Bad firmware image (bad FIT header)\n"); > return -EINVAL; > } > diff --git a/drivers/net/pfe_eth/pfe_firmware.c b/drivers/net/pfe_eth/pfe_firmware.c > index 41999e176d4..eee70a2e73a 100644 > --- a/drivers/net/pfe_eth/pfe_firmware.c > +++ b/drivers/net/pfe_eth/pfe_firmware.c > @@ -160,7 +160,7 @@ static int pfe_fit_check(void) > return ret; > } > > - if (!fit_check_format(pfe_fit_addr)) { > + if (fit_check_format(pfe_fit_addr, IMAGE_SIZE_INVAL)) { > printf("PFE Firmware: Bad firmware image (bad FIT header)\n"); > ret = -1; > return ret; > diff --git a/include/image.h b/include/image.h > index 856bc3e1b29..d5a940313a6 100644 > --- a/include/image.h > +++ b/include/image.h > @@ -134,6 +134,9 @@ extern ulong image_load_addr; /* Default Load Address */ > extern ulong image_save_addr; /* Default Save Address */ > extern ulong image_save_size; /* Default Save Size */ > > +/* An invalid size, meaning that the image size is not known */ > +#define IMAGE_SIZE_INVAL (-1UL) > + > enum ih_category { > IH_ARCH, > IH_COMP, > @@ -1142,7 +1145,23 @@ int fit_image_check_os(const void *fit, int noffset, uint8_t os); > int fit_image_check_arch(const void *fit, int noffset, uint8_t arch); > int fit_image_check_type(const void *fit, int noffset, uint8_t type); > int fit_image_check_comp(const void *fit, int noffset, uint8_t comp); > -int fit_check_format(const void *fit); > + > +/** > + * fit_check_format() - Check that the FIT is valid > + * > + * This performs various checks on the FIT to make sure it is suitable for > + * use, looking for mandatory properties, nodes, etc. > + * > + * If FIT_FULL_CHECK is enabled, it also runs it through libfdt to make > + * sure that there are no strange tags or broken nodes in the FIT. > + * > + * @fit: pointer to the FIT format image header > + * @return 0 if OK, -ENOEXEC if not an FDT file, -EINVAL if the full FDT check > + * failed (e.g. due to bad structure), -ENOMSG if the description is > + * missing, -ENODATA if the timestamp is missing, -ENOENT if the /images > + * path is missing > + */ > +int fit_check_format(const void *fit, ulong size); > > int fit_conf_find_compat(const void *fit, const void *fdt); > > diff --git a/tools/fit_common.c b/tools/fit_common.c > index cdf987d3c13..52b63296f89 100644 > --- a/tools/fit_common.c > +++ b/tools/fit_common.c > @@ -26,7 +26,8 @@ > int fit_verify_header(unsigned char *ptr, int image_size, > struct image_tool_params *params) > { > - if (fdt_check_header(ptr) != EXIT_SUCCESS || !fit_check_format(ptr)) > + if (fdt_check_header(ptr) != EXIT_SUCCESS || > + fit_check_format(ptr, IMAGE_SIZE_INVAL)) > return EXIT_FAILURE; > > return EXIT_SUCCESS; > diff --git a/tools/fit_image.c b/tools/fit_image.c > index 06faeda7c26..d440d143c67 100644 > --- a/tools/fit_image.c > +++ b/tools/fit_image.c > @@ -883,7 +883,7 @@ static int fit_extract_contents(void *ptr, struct image_tool_params *params) > /* Indent string is defined in header image.h */ > p = IMAGE_INDENT_STRING; > > - if (!fit_check_format(fit)) { > + if (fit_check_format(fit, IMAGE_SIZE_INVAL)) { > printf("Bad FIT image format\n"); > return -1; > } > diff --git a/tools/mkimage.h b/tools/mkimage.h > index 5b096a545b7..0d3148444c3 100644 > --- a/tools/mkimage.h > +++ b/tools/mkimage.h > @@ -29,6 +29,8 @@ > #define debug(fmt,args...) > #endif /* MKIMAGE_DEBUG */ > > +#define log_debug(fmt, args...) debug(fmt, ##args) > + > static inline void *map_sysmem(ulong paddr, unsigned long len) > { > return (void *)(uintptr_t)paddr;