[PATCH 5/8] image: Adjust the workings of fit_check_format()
Jesper Schmitz Mouridsen
jesper at schmitz.computer
Wed Feb 17 14:30:56 CET 2021
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 <sjg at chromium.org>
> Reported-by: Bruce Monroe <bruce.monroe at intel.com>
> Reported-by: Arie Haenel <arie.haenel at intel.com>
> Reported-by: Julien Lenoir <julien.lenoir at intel.com>
> ---
>
> 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 <time.h>
> @@ -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;
More information about the U-Boot
mailing list