From: Simon Glass <sjg@chromium.org>
To: u-boot@lists.denx.de
Subject: [PATCH 5/8] image: Adjust the workings of fit_check_format()
Date: Mon, 15 Feb 2021 17:08:09 -0700 [thread overview]
Message-ID: <20210216000812.2091481-6-sjg@chromium.org> (raw)
In-Reply-To: <20210216000812.2091481-1-sjg@chromium.org>
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@chromium.org>
Reported-by: Bruce Monroe <bruce.monroe@intel.com>
Reported-by: Arie Haenel <arie.haenel@intel.com>
Reported-by: Julien Lenoir <julien.lenoir@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@%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;
--
2.30.0.478.g8a0d178c01-goog
next prev parent reply other threads:[~2021-02-16 0:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-16 0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
2021-02-16 0:08 ` [PATCH 1/8] fdt_region: Check for a single root node of the correct name Simon Glass
2021-02-16 3:35 ` Tom Rini
2021-02-16 0:08 ` [PATCH 2/8] fit: Don't allow verification of images with @ nodes Simon Glass
2021-02-16 3:35 ` Tom Rini
2021-02-16 0:08 ` [PATCH 3/8] test: Add vboot_evil implementation Simon Glass
2021-02-16 3:36 ` Tom Rini
2021-02-16 0:08 ` [PATCH 4/8] test: Add tests for the 'evil' vboot attacks Simon Glass
2021-02-16 3:36 ` Tom Rini
2021-02-16 0:08 ` Simon Glass [this message]
2021-02-16 3:36 ` [PATCH 5/8] image: Adjust the workings of fit_check_format() Tom Rini
2021-02-17 13:30 ` Jesper Schmitz Mouridsen
2021-02-17 13:43 ` Tom Rini
2021-02-17 14:12 ` Jesper Schmitz Mouridsen
2021-02-16 0:08 ` [PATCH 6/8] image: Add an option to do a full check of the FIT Simon Glass
2021-02-16 3:36 ` Tom Rini
2021-02-16 0:08 ` [PATCH 7/8] libfdt: Check for multiple/invalid root nodes Simon Glass
2021-02-16 3:36 ` Tom Rini
2021-02-16 0:08 ` [PATCH 8/8] image: Check for unit addresses in FITs Simon Glass
2021-02-16 3:36 ` Tom Rini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210216000812.2091481-6-sjg@chromium.org \
--to=sjg@chromium.org \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.