All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel
@ 2021-02-16  0:08 Simon Glass
  2021-02-16  0:08 ` [PATCH 1/8] fdt_region: Check for a single root node of the correct name Simon Glass
                   ` (7 more replies)
  0 siblings, 8 replies; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot


This series fixes some vulnerabilities in U-Boot identified by:

    Julien Lenoir <julien.lenoir@intel.com>
    Bruce Monroe <bruce.monroe@intel.com>
    Arie Haenel <arie.haenel@intel.com>

First problem
-------------
CVE ID - CVE-2021-27097
Reference URL - http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-27097
Note this CVE ID will so reserved until a few days after publication.

CVE Description:
Improper input validation in U-Boot boot loader before version 2021.04-rc2
may allow an authenticated user escalate privileges via local access.

CVSS Base Score: 7.8 High
CVSS Vector - https://www.first.org/cvss/calculator/3.1#CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H

U-Boot fetches the signature from the "/configuration" node and later the
code from the "/images" node, once signature is verified.

The list of nodes to hash is stored in the 'hashed-nodes' property of the
signature node:

   hashed-nodes = b'/\x00/configurations/conf at 1\x00
             /images/kernel at 1\x00/images/kernel at 1/hash at 1\x00'

Signature is computed on memory blocks fetched by the
fdt_find_regions_function() function. This matches the blocks based on
their 'path', based on the nodes/prop names of the tree.

The problem is: the fact that the first node is the root, i.e. a node with
an empty name, seems to be a convention that UBoot assumes to be true, but
it does not check it explicitly.

It is possible to build a FIT in which the first node has a non-empty
name, for example 'f at kenode' followed by a genuine root node (with an
empty name). U-Boot will then fetch the configuration and images from the
fake node, thus fetching from '/f at kenode/configuration' and
'/f at kenode/images".

Contrarywise, the fdt_find_regions_function() function, matches nodes on
their full path, without this assumption.

As a result: signature is checked on the 'real' root node while code is
loaded from the fake one.

Second problem
--------------
CVE ID - CVE-2021-27138

CVE Description:

Improper input validation in Das U-Boot before version 2020.04-rc2 may
allow an authenticated user escalate privileges via local access.

CVSS Base Score: 7.8 High

CVSS Vector - https://www.first.org/cvss/calculator/3.1#CVSS:3.1/AV:L/AC:L/PR:N/UI:R/S:U/C:H/I:H/A:H

CVE ID - CVE-2021-27138 - Reference URL - http://cve.mitre.org/cgi-bin/cvename.cgi?name= CVE-2021-27138. Note this CVE ID will so reserved until a few days after publication.

A second problem is noticed when an @ symbol is added to a node that does
not already have one. In 2017 U-Boot moved away from using @ in node names
due to the devicetree compiler warning about them, e.g. in this commit:

 838404054e4 ("doc: FIT image: fix incorrect description of DT node unit address")

This means that it is possible to add a node name, like fdt-1 at evil before
the existing fdt-1 node, and U-Boot will use the first one, due to the way
the unit-address matching works. Of course, people may still be using the
older @ nodes and thus avoiding this problem, but the examples were
updated to use a hyphen so this is unlikely.

This series corrects the above vulnerabilities.


Simon Glass (8):
  fdt_region: Check for a single root node of the correct name
  fit: Don't allow verification of images with @ nodes
  test: Add vboot_evil implementation
  test: Add tests for the 'evil' vboot attacks
  image: Adjust the workings of fit_check_format()
  image: Add an option to do a full check of the FIT
  libfdt: Check for multiple/invalid root nodes
  image: Check for unit addresses in FITs

 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/Kconfig.boot                |  20 ++
 common/fdt_region.c                |  11 +
 common/image-fdt.c                 |   2 +-
 common/image-fit-sig.c             |  22 +-
 common/image-fit.c                 | 126 ++++++--
 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 +-
 scripts/dtc/libfdt/fdt_ro.c        |  17 +
 test/py/tests/test_fit.py          |  24 +-
 test/py/tests/test_vboot.py        |  95 ++++--
 test/py/tests/vboot_evil.py        | 485 +++++++++++++++++++++++++++++
 test/py/tests/vboot_forge.py       |  12 +-
 tools/fit_common.c                 |   3 +-
 tools/fit_image.c                  |   2 +-
 tools/mkimage.h                    |   2 +
 27 files changed, 781 insertions(+), 101 deletions(-)
 create mode 100644 test/py/tests/vboot_evil.py

-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/8] fdt_region: Check for a single root node of the correct name
  2021-02-16  0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
@ 2021-02-16  0:08 ` 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
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

At present fdt_find_regions() assumes that the FIT is a valid devicetree.
If the FIT has two root nodes this is currently not detected in this
function, nor does libfdt's fdt_check_full() notice. Also it is possible
for the root node to have a name even though it should not.

Add checks for these and return -FDT_ERR_BADSTRUCTURE if a problem is
detected.

CVE-2021-27097

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>
---

 common/fdt_region.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/common/fdt_region.c b/common/fdt_region.c
index ff12c518e97..e4ef0ca7703 100644
--- a/common/fdt_region.c
+++ b/common/fdt_region.c
@@ -43,6 +43,7 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
 	int depth = -1;
 	int want = 0;
 	int base = fdt_off_dt_struct(fdt);
+	bool expect_end = false;
 
 	end = path;
 	*end = '\0';
@@ -59,6 +60,10 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
 		tag = fdt_next_tag(fdt, offset, &nextoffset);
 		stop_at = nextoffset;
 
+		/* If we see two root nodes, something is wrong */
+		if (expect_end && tag != FDT_END)
+			return -FDT_ERR_BADLAYOUT;
+
 		switch (tag) {
 		case FDT_PROP:
 			include = want >= 2;
@@ -81,6 +86,10 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
 			if (depth == FDT_MAX_DEPTH)
 				return -FDT_ERR_BADSTRUCTURE;
 			name = fdt_get_name(fdt, offset, &len);
+
+			/* The root node must have an empty name */
+			if (!depth && *name)
+				return -FDT_ERR_BADLAYOUT;
 			if (end - path + 2 + len >= path_len)
 				return -FDT_ERR_NOSPACE;
 			if (end != path + 1)
@@ -108,6 +117,8 @@ int fdt_find_regions(const void *fdt, char * const inc[], int inc_count,
 			while (end > path && *--end != '/')
 				;
 			*end = '\0';
+			if (depth == -1)
+				expect_end = true;
 			break;
 
 		case FDT_END:
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/8] fit: Don't allow verification of images with @ nodes
  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  0:08 ` Simon Glass
  2021-02-16  3:35   ` Tom Rini
  2021-02-16  0:08 ` [PATCH 3/8] test: Add vboot_evil implementation Simon Glass
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

When searching for a node called 'fred', any unit address appended to the
name is ignored by libfdt, meaning that 'fred' can match 'fred at 1'. This
means that we cannot be sure that the node originally intended is the one
that is used.

Disallow use of nodes with unit addresses.

Update the forge test also, since it uses @ addresses.

CVE-2021-27138

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>
---

 common/image-fit-sig.c       | 22 ++++++++++++++++++++--
 common/image-fit.c           | 20 +++++++++++++++-----
 test/py/tests/test_fit.py    | 24 ++++++++++++------------
 test/py/tests/vboot_forge.py | 12 ++++++------
 4 files changed, 53 insertions(+), 25 deletions(-)

diff --git a/common/image-fit-sig.c b/common/image-fit-sig.c
index 897e04c7a38..34ebb8edfe2 100644
--- a/common/image-fit-sig.c
+++ b/common/image-fit-sig.c
@@ -149,6 +149,14 @@ static int fit_image_verify_sig(const void *fit, int image_noffset,
 	fdt_for_each_subnode(noffset, fit, image_noffset) {
 		const char *name = fit_get_name(fit, noffset, NULL);
 
+		/*
+		 * We don't support this since libfdt considers names with the
+		 * name root but different @ suffix to be equal
+		 */
+		if (strchr(name, '@')) {
+			err_msg = "Node name contains @";
+			goto error;
+		}
 		if (!strncmp(name, FIT_SIG_NODENAME,
 			     strlen(FIT_SIG_NODENAME))) {
 			ret = fit_image_check_sig(fit, noffset, data,
@@ -398,9 +406,10 @@ error:
 	return -EPERM;
 }
 
-int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
-				    const void *sig_blob)
+static int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
+					   const void *sig_blob)
 {
+	const char *name = fit_get_name(fit, conf_noffset, NULL);
 	int noffset;
 	int sig_node;
 	int verified = 0;
@@ -408,6 +417,15 @@ int fit_config_verify_required_sigs(const void *fit, int conf_noffset,
 	bool reqd_policy_all = true;
 	const char *reqd_mode;
 
+	/*
+	 * We don't support this since libfdt considers names with the
+	 * name root but different @ suffix to be equal
+	 */
+	if (strchr(name, '@')) {
+		printf("Configuration node '%s' contains '@'\n", name);
+		return -EPERM;
+	}
+
 	/* Work out what we need to verify */
 	sig_node = fdt_subnode_offset(sig_blob, 0, FIT_SIG_NODENAME);
 	if (sig_node < 0) {
diff --git a/common/image-fit.c b/common/image-fit.c
index adc3e551de9..c3dc814115f 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1369,21 +1369,31 @@ error:
  */
 int fit_image_verify(const void *fit, int image_noffset)
 {
+	const char *name = fit_get_name(fit, image_noffset, NULL);
 	const void	*data;
 	size_t		size;
-	int		noffset = 0;
 	char		*err_msg = "";
 
+	if (strchr(name, '@')) {
+		/*
+		 * We don't support this since libfdt considers names with the
+		 * name root but different @ suffix to be equal
+		 */
+		err_msg = "Node name contains @";
+		goto err;
+	}
 	/* Get image data and data length */
 	if (fit_image_get_data_and_size(fit, image_noffset, &data, &size)) {
 		err_msg = "Can't get image data/size";
-		printf("error!\n%s for '%s' hash node in '%s' image node\n",
-		       err_msg, fit_get_name(fit, noffset, NULL),
-		       fit_get_name(fit, image_noffset, NULL));
-		return 0;
+		goto err;
 	}
 
 	return fit_image_verify_with_data(fit, image_noffset, data, size);
+
+err:
+	printf("error!\n%s in '%s' image node\n", err_msg,
+	       fit_get_name(fit, image_noffset, NULL));
+	return 0;
 }
 
 /**
diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py
index 84b3f958505..6d5b43c3bab 100755
--- a/test/py/tests/test_fit.py
+++ b/test/py/tests/test_fit.py
@@ -17,7 +17,7 @@ base_its = '''
         #address-cells = <1>;
 
         images {
-                kernel at 1 {
+                kernel-1 {
                         data = /incbin/("%(kernel)s");
                         type = "kernel";
                         arch = "sandbox";
@@ -26,7 +26,7 @@ base_its = '''
                         load = <0x40000>;
                         entry = <0x8>;
                 };
-                kernel at 2 {
+                kernel-2 {
                         data = /incbin/("%(loadables1)s");
                         type = "kernel";
                         arch = "sandbox";
@@ -35,19 +35,19 @@ base_its = '''
                         %(loadables1_load)s
                         entry = <0x0>;
                 };
-                fdt at 1 {
+                fdt-1 {
                         description = "snow";
                         data = /incbin/("%(fdt)s");
                         type = "flat_dt";
                         arch = "sandbox";
                         %(fdt_load)s
                         compression = "%(compression)s";
-                        signature at 1 {
+                        signature-1 {
                                 algo = "sha1,rsa2048";
                                 key-name-hint = "dev";
                         };
                 };
-                ramdisk at 1 {
+                ramdisk-1 {
                         description = "snow";
                         data = /incbin/("%(ramdisk)s");
                         type = "ramdisk";
@@ -56,7 +56,7 @@ base_its = '''
                         %(ramdisk_load)s
                         compression = "%(compression)s";
                 };
-                ramdisk at 2 {
+                ramdisk-2 {
                         description = "snow";
                         data = /incbin/("%(loadables2)s");
                         type = "ramdisk";
@@ -67,10 +67,10 @@ base_its = '''
                 };
         };
         configurations {
-                default = "conf at 1";
-                conf at 1 {
-                        kernel = "kernel at 1";
-                        fdt = "fdt at 1";
+                default = "conf-1";
+                conf-1 {
+                        kernel = "kernel-1";
+                        fdt = "fdt-1";
                         %(ramdisk_config)s
                         %(loadables_config)s
                 };
@@ -410,7 +410,7 @@ def test_fit(u_boot_console):
 
         # Try a ramdisk
         with cons.log.section('Kernel + FDT + Ramdisk load'):
-            params['ramdisk_config'] = 'ramdisk = "ramdisk at 1";'
+            params['ramdisk_config'] = 'ramdisk = "ramdisk-1";'
             params['ramdisk_load'] = 'load = <%#x>;' % params['ramdisk_addr']
             fit = make_fit(mkimage, params)
             cons.restart_uboot()
@@ -419,7 +419,7 @@ def test_fit(u_boot_console):
 
         # Configuration with some Loadables
         with cons.log.section('Kernel + FDT + Ramdisk load + Loadables'):
-            params['loadables_config'] = 'loadables = "kernel at 2", "ramdisk at 2";'
+            params['loadables_config'] = 'loadables = "kernel-2", "ramdisk-2";'
             params['loadables1_load'] = ('load = <%#x>;' %
                                          params['loadables1_addr'])
             params['loadables2_load'] = ('load = <%#x>;' %
diff --git a/test/py/tests/vboot_forge.py b/test/py/tests/vboot_forge.py
index 0fb7ef40247..b41105bd0e3 100644
--- a/test/py/tests/vboot_forge.py
+++ b/test/py/tests/vboot_forge.py
@@ -376,12 +376,12 @@ def manipulate(root, strblock):
     """
     Maliciously manipulates the structure to create a crafted FIT file
     """
-    # locate /images/kernel at 1 (frankly, it just expects it to be the first one)
+    # locate /images/kernel-1 (frankly, it just expects it to be the first one)
     kernel_node = root[0][0]
     # clone it to save time filling all the properties
     fake_kernel = kernel_node.clone()
     # rename the node
-    fake_kernel.name = b'kernel at 2'
+    fake_kernel.name = b'kernel-2'
     # get rid of signatures/hashes
     fake_kernel.children = []
     # NOTE: this simply replaces the first prop... either description or data
@@ -391,13 +391,13 @@ def manipulate(root, strblock):
     root[0].children.append(fake_kernel)
 
     # modify the default configuration
-    root[1].props[0].value = b'conf at 2\x00'
+    root[1].props[0].value = b'conf-2\x00'
     # clone the first (only?) configuration
     fake_conf = root[1][0].clone()
     # rename and change kernel and fdt properties to select the crafted kernel
-    fake_conf.name = b'conf at 2'
-    fake_conf.props[0].value = b'kernel at 2\x00'
-    fake_conf.props[1].value = b'fdt at 1\x00'
+    fake_conf.name = b'conf-2'
+    fake_conf.props[0].value = b'kernel-2\x00'
+    fake_conf.props[1].value = b'fdt-1\x00'
     # insert the new configuration under /configurations
     root[1].children.append(fake_conf)
 
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/8] test: Add vboot_evil implementation
  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  0:08 ` [PATCH 2/8] fit: Don't allow verification of images with @ nodes Simon Glass
@ 2021-02-16  0:08 ` 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
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

Add a library which performs two different attacks on a FIT.

Signed-off-by: Julien Lenoir <julien.lenoir@intel.com>
Signed-off-by: Bruce Monroe <bruce.monroe@intel.com>
Signed-off-by: Arie Haenel <arie.haenel@intel.com>
Signed-off-by: Simon Glass <sjg@chromium.org>
---

 test/py/tests/vboot_evil.py | 485 ++++++++++++++++++++++++++++++++++++
 1 file changed, 485 insertions(+)
 create mode 100644 test/py/tests/vboot_evil.py

diff --git a/test/py/tests/vboot_evil.py b/test/py/tests/vboot_evil.py
new file mode 100644
index 00000000000..9825c21716b
--- /dev/null
+++ b/test/py/tests/vboot_evil.py
@@ -0,0 +1,485 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020, Intel Corporation
+
+"""Modifies a devicetree to add a fake root node, for testing purposes"""
+
+import hashlib
+import struct
+import sys
+
+FDT_PROP = 0x3
+FDT_BEGIN_NODE = 0x1
+FDT_END_NODE = 0x2
+FDT_END = 0x9
+
+FAKE_ROOT_ATTACK = 0
+KERNEL_AT = 1
+
+MAGIC = 0xd00dfeed
+
+EVIL_KERNEL_NAME = b'evil_kernel'
+FAKE_ROOT_NAME = b'f at keroot'
+
+
+def getstr(dt_strings, off):
+    """Get a string from the devicetree string table
+
+    Args:
+        dt_strings (bytes): Devicetree strings section
+        off (int): Offset of string to read
+
+    Returns:
+        str: String read from the table
+    """
+    output = ''
+    while dt_strings[off]:
+        output += chr(dt_strings[off])
+        off += 1
+
+    return output
+
+
+def align(offset):
+    """Align an offset to a multiple of 4
+
+    Args:
+        offset (int): Offset to align
+
+    Returns:
+        int: Resulting aligned offset (rounds up to nearest multiple)
+    """
+    return (offset + 3) & ~3
+
+
+def determine_offset(dt_struct, dt_strings, searched_node_name):
+    """Determines the offset of an element, either a node or a property
+
+    Args:
+        dt_struct (bytes): Devicetree struct section
+        dt_strings (bytes): Devicetree strings section
+        searched_node_name (str): element path, ex: /images/kernel at 1/data
+
+    Returns:
+        tuple: (node start offset, node end offset)
+        if element is not found, returns (None, None)
+    """
+    offset = 0
+    depth = -1
+
+    path = '/'
+
+    object_start_offset = None
+    object_end_offset = None
+    object_depth = None
+
+    while offset < len(dt_struct):
+        (tag,) = struct.unpack('>I', dt_struct[offset:offset + 4])
+
+        if tag == FDT_BEGIN_NODE:
+            depth += 1
+
+            begin_node_offset = offset
+            offset += 4
+
+            node_name = getstr(dt_struct, offset)
+            offset += len(node_name) + 1
+            offset = align(offset)
+
+            if path[-1] != '/':
+                path += '/'
+
+            path += str(node_name)
+
+            if path == searched_node_name:
+                object_start_offset = begin_node_offset
+                object_depth = depth
+
+        elif tag == FDT_PROP:
+            begin_prop_offset = offset
+
+            offset += 4
+            len_tag, nameoff = struct.unpack('>II',
+                                             dt_struct[offset:offset + 8])
+            offset += 8
+            prop_name = getstr(dt_strings, nameoff)
+
+            len_tag = align(len_tag)
+
+            offset += len_tag
+
+            node_path = path + '/' + str(prop_name)
+
+            if node_path == searched_node_name:
+                object_start_offset = begin_prop_offset
+
+        elif tag == FDT_END_NODE:
+            offset += 4
+
+            path = path[:path.rfind('/')]
+            if not path:
+                path = '/'
+
+            if depth == object_depth:
+                object_end_offset = offset
+                break
+            depth -= 1
+        elif tag == FDT_END:
+            break
+
+        else:
+            print('unknown tag=0x%x, offset=0x%x found!' % (tag, offset))
+            break
+
+    return object_start_offset, object_end_offset
+
+
+def modify_node_name(dt_struct, node_offset, replcd_name):
+    """Change the name of a node
+
+    Args:
+        dt_struct (bytes): Devicetree struct section
+        node_offset (int): Offset of node
+        replcd_name (str): New name for node
+
+    Returns:
+        bytes: New dt_struct contents
+    """
+
+    # skip 4 bytes for the FDT_BEGIN_NODE
+    node_offset += 4
+
+    node_name = getstr(dt_struct, node_offset)
+    node_name_len = len(node_name) + 1
+
+    node_name_len = align(node_name_len)
+
+    replcd_name += b'\0'
+
+    # align on 4 bytes
+    while len(replcd_name) % 4:
+        replcd_name += b'\0'
+
+    dt_struct = (dt_struct[:node_offset] + replcd_name +
+                 dt_struct[node_offset + node_name_len:])
+
+    return dt_struct
+
+
+def modify_prop_content(dt_struct, prop_offset, content):
+    """Overwrite the value of a property
+
+    Args:
+        dt_struct (bytes): Devicetree struct section
+        prop_offset (int): Offset of property (FDT_PROP tag)
+        content (bytes): New content for the property
+
+    Returns:
+        bytes: New dt_struct contents
+    """
+    # skip FDT_PROP
+    prop_offset += 4
+    (len_tag, nameoff) = struct.unpack('>II',
+                                       dt_struct[prop_offset:prop_offset + 8])
+
+    # compute padded original node length
+    original_node_len = len_tag + 8  # content length + prop meta data len
+
+    original_node_len = align(original_node_len)
+
+    added_data = struct.pack('>II', len(content), nameoff)
+    added_data += content
+    while len(added_data) % 4:
+        added_data += b'\0'
+
+    dt_struct = (dt_struct[:prop_offset] + added_data +
+                 dt_struct[prop_offset + original_node_len:])
+
+    return dt_struct
+
+
+def change_property_value(dt_struct, dt_strings, prop_path, prop_value,
+                          required=True):
+    """Change a given property value
+
+    Args:
+        dt_struct (bytes): Devicetree struct section
+        dt_strings (bytes): Devicetree strings section
+        prop_path (str): full path of the target property
+        prop_value (bytes):  new property name
+        required (bool): raise an exception if property not found
+
+    Returns:
+        bytes: New dt_struct contents
+
+    Raises:
+        ValueError: if the property is not found
+    """
+    (rt_node_start, _) = determine_offset(dt_struct, dt_strings, prop_path)
+    if rt_node_start is None:
+        if not required:
+            return dt_struct
+        raise ValueError('Fatal error, unable to find prop %s' % prop_path)
+
+    dt_struct = modify_prop_content(dt_struct, rt_node_start, prop_value)
+
+    return dt_struct
+
+def change_node_name(dt_struct, dt_strings, node_path, node_name):
+    """Change a given node name
+
+    Args:
+        dt_struct (bytes): Devicetree struct section
+        dt_strings (bytes): Devicetree strings section
+        node_path (str): full path of the target node
+        node_name (str): new node name, just node name not full path
+
+    Returns:
+        bytes: New dt_struct contents
+
+    Raises:
+        ValueError: if the node is not found
+    """
+    (rt_node_start, rt_node_end) = (
+        determine_offset(dt_struct, dt_strings, node_path))
+    if rt_node_start is None or rt_node_end is None:
+        raise ValueError('Fatal error, unable to find root node')
+
+    dt_struct = modify_node_name(dt_struct, rt_node_start, node_name)
+
+    return dt_struct
+
+def get_prop_value(dt_struct, dt_strings, prop_path):
+    """Get the content of a property based on its path
+
+    Args:
+        dt_struct (bytes): Devicetree struct section
+        dt_strings (bytes): Devicetree strings section
+        prop_path (str): full path of the target property
+
+    Returns:
+        bytes: Property value
+
+    Raises:
+        ValueError: if the property is not found
+    """
+    (offset, _) = determine_offset(dt_struct, dt_strings, prop_path)
+    if offset is None:
+        raise ValueError('Fatal error, unable to find prop')
+
+    offset += 4
+    (len_tag,) = struct.unpack('>I', dt_struct[offset:offset + 4])
+
+    offset += 8
+    tag_data = dt_struct[offset:offset + len_tag]
+
+    return tag_data
+
+
+def kernel_at_attack(dt_struct, dt_strings, kernel_content, kernel_hash):
+    """Conduct the kernel@ attack
+
+    It fetches from /configurations/default the name of the kernel being loaded.
+    Then, if the kernel name does not contain any @sign, duplicates the kernel
+    in /images node and appends '@evil' to its name.
+    It inserts a new kernel content and updates its images digest.
+
+    Inputs:
+        - FIT dt_struct
+        - FIT dt_strings
+        - kernel content blob
+        - kernel hash blob
+
+    Important note: it assumes the U-Boot loading method is 'kernel' and the
+    loaded kernel hash's subnode name is 'hash-1'
+    """
+
+    # retrieve the default configuration name
+    default_conf_name = get_prop_value(
+        dt_struct, dt_strings, '/configurations/default')
+    default_conf_name = str(default_conf_name[:-1], 'utf-8')
+
+    conf_path = '/configurations/' + default_conf_name
+
+    # fetch the loaded kernel name from the default configuration
+    loaded_kernel = get_prop_value(dt_struct, dt_strings, conf_path + '/kernel')
+
+    loaded_kernel = str(loaded_kernel[:-1], 'utf-8')
+
+    if loaded_kernel.find('@') != -1:
+        print('kernel@ attack does not work on nodes already containing an @ sign!')
+        sys.exit()
+
+    # determine boundaries of the loaded kernel
+    (krn_node_start, krn_node_end) = (determine_offset(
+        dt_struct, dt_strings, '/images/' + loaded_kernel))
+    if krn_node_start is None and krn_node_end is None:
+        print('Fatal error, unable to find root node')
+        sys.exit()
+
+    # copy the loaded kernel
+    loaded_kernel_copy = dt_struct[krn_node_start:krn_node_end]
+
+    # insert the copy inside the tree
+    dt_struct = dt_struct[:krn_node_start] + \
+        loaded_kernel_copy + dt_struct[krn_node_start:]
+
+    evil_kernel_name = loaded_kernel+'@evil'
+
+    # change the inserted kernel name
+    dt_struct = change_node_name(
+        dt_struct, dt_strings, '/images/' + loaded_kernel, bytes(evil_kernel_name, 'utf-8'))
+
+    # change the content of the kernel being loaded
+    dt_struct = change_property_value(
+        dt_struct, dt_strings, '/images/' + evil_kernel_name + '/data', kernel_content)
+
+    # change the content of the kernel being loaded
+    dt_struct = change_property_value(
+        dt_struct, dt_strings, '/images/' + evil_kernel_name + '/hash-1/value', kernel_hash)
+
+    return dt_struct
+
+
+def fake_root_node_attack(dt_struct, dt_strings, kernel_content, kernel_digest):
+    """Conduct the fakenode attack
+
+    It duplicates the original root node at the beginning of the tree.
+    Then it modifies within this duplicated tree:
+        - The loaded kernel name
+        - The loaded  kernel data
+
+    Important note: it assumes the UBoot loading method is 'kernel' and the loaded kernel
+    hash's subnode name is hash at 1
+    """
+
+    # retrieve the default configuration name
+    default_conf_name = get_prop_value(
+        dt_struct, dt_strings, '/configurations/default')
+    default_conf_name = str(default_conf_name[:-1], 'utf-8')
+
+    conf_path = '/configurations/'+default_conf_name
+
+    # fetch the loaded kernel name from the default configuration
+    loaded_kernel = get_prop_value(dt_struct, dt_strings, conf_path + '/kernel')
+
+    loaded_kernel = str(loaded_kernel[:-1], 'utf-8')
+
+    # determine root node start and end:
+    (rt_node_start, rt_node_end) = (determine_offset(dt_struct, dt_strings, '/'))
+    if (rt_node_start is None) or (rt_node_end is None):
+        print('Fatal error, unable to find root node')
+        sys.exit()
+
+    # duplicate the whole tree
+    duplicated_node = dt_struct[rt_node_start:rt_node_end]
+
+    # dchange root name (empty name) to fake root name
+    new_dup = change_node_name(duplicated_node, dt_strings, '/', FAKE_ROOT_NAME)
+
+    dt_struct = new_dup + dt_struct
+
+    # change the value of /<fake_root_name>/configs/<default_config_name>/kernel
+    # so our modified kernel will be loaded
+    base = '/' + str(FAKE_ROOT_NAME, 'utf-8')
+    value_path = base + conf_path+'/kernel'
+    dt_struct = change_property_value(dt_struct, dt_strings, value_path,
+                                      EVIL_KERNEL_NAME + b'\0')
+
+    # change the node of the /<fake_root_name>/images/<original_kernel_name>
+    images_path = base + '/images/'
+    node_path = images_path + loaded_kernel
+    dt_struct = change_node_name(dt_struct, dt_strings, node_path,
+                                 EVIL_KERNEL_NAME)
+
+    # change the content of the kernel being loaded
+    data_path = images_path + str(EVIL_KERNEL_NAME, 'utf-8') + '/data'
+    dt_struct = change_property_value(dt_struct, dt_strings, data_path,
+                                      kernel_content, required=False)
+
+    # update the digest value
+    hash_path = images_path + str(EVIL_KERNEL_NAME, 'utf-8') + '/hash-1/value'
+    dt_struct = change_property_value(dt_struct, dt_strings, hash_path,
+                                      kernel_digest)
+
+    return dt_struct
+
+def add_evil_node(in_fname, out_fname, kernel_fname, attack):
+    """Add an evil node to the devicetree
+
+    Args:
+        in_fname (str): Filename of input devicetree
+        out_fname (str): Filename to write modified devicetree to
+        kernel_fname (str): Filename of kernel data to add to evil node
+        attack (str): Attack type ('fakeroot' or 'kernel@')
+
+    Raises:
+        ValueError: Unknown attack name
+    """
+    if attack == 'fakeroot':
+        attack = FAKE_ROOT_ATTACK
+    elif attack == 'kernel@':
+        attack = KERNEL_AT
+    else:
+        raise ValueError('Unknown attack name!')
+
+    with open(in_fname, 'rb') as fin:
+        input_data = fin.read()
+
+    hdr = input_data[0:0x28]
+
+    offset = 0
+    magic = struct.unpack('>I', hdr[offset:offset + 4])[0]
+    if magic != MAGIC:
+        raise ValueError('Wrong magic!')
+
+    offset += 4
+    (totalsize, off_dt_struct, off_dt_strings, off_mem_rsvmap, version,
+     last_comp_version, boot_cpuid_phys, size_dt_strings,
+     size_dt_struct) = struct.unpack('>IIIIIIIII', hdr[offset:offset + 36])
+
+    rsv_map = input_data[off_mem_rsvmap:off_dt_struct]
+    dt_struct = input_data[off_dt_struct:off_dt_struct + size_dt_struct]
+    dt_strings = input_data[off_dt_strings:off_dt_strings + size_dt_strings]
+
+    with open(kernel_fname, 'rb') as kernel_file:
+        kernel_content = kernel_file.read()
+
+    # computing inserted kernel hash
+    val = hashlib.sha1()
+    val.update(kernel_content)
+    hash_digest = val.digest()
+
+    if attack == FAKE_ROOT_ATTACK:
+        dt_struct = fake_root_node_attack(dt_struct, dt_strings, kernel_content,
+                                          hash_digest)
+    elif attack == KERNEL_AT:
+        dt_struct = kernel_at_attack(dt_struct, dt_strings, kernel_content,
+                                     hash_digest)
+
+    # now rebuild the new file
+    size_dt_strings = len(dt_strings)
+    size_dt_struct = len(dt_struct)
+    totalsize = 0x28 + len(rsv_map) + size_dt_struct + size_dt_strings
+    off_mem_rsvmap = 0x28
+    off_dt_struct = off_mem_rsvmap + len(rsv_map)
+    off_dt_strings = off_dt_struct + len(dt_struct)
+
+    header = struct.pack('>IIIIIIIIII', MAGIC, totalsize, off_dt_struct,
+                         off_dt_strings, off_mem_rsvmap, version,
+                         last_comp_version, boot_cpuid_phys, size_dt_strings,
+                         size_dt_struct)
+
+    with open(out_fname, 'wb') as output_file:
+        output_file.write(header)
+        output_file.write(rsv_map)
+        output_file.write(dt_struct)
+        output_file.write(dt_strings)
+
+if __name__ == '__main__':
+    if len(sys.argv) != 5:
+        print('usage: %s <input_filename> <output_filename> <kernel_binary> <attack_name>' %
+              sys.argv[0])
+        print('valid attack names: [fakeroot, kernel@]')
+        sys.exit(1)
+
+    add_evil_node(sys.argv[1:])
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 4/8] test: Add tests for the 'evil' vboot attacks
  2021-02-16  0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
                   ` (2 preceding siblings ...)
  2021-02-16  0:08 ` [PATCH 3/8] test: Add vboot_evil implementation Simon Glass
@ 2021-02-16  0:08 ` Simon Glass
  2021-02-16  3:36   ` Tom Rini
  2021-02-16  0:08 ` [PATCH 5/8] image: Adjust the workings of fit_check_format() Simon Glass
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

Add tests to check that these two attacks are mitigated by recent patches.

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>
---

 test/py/tests/test_vboot.py | 93 ++++++++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 28 deletions(-)

diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index e45800d94c0..9cdc290b9fc 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -24,22 +24,26 @@ For configuration verification:
 Tests run with both SHA1 and SHA256 hashing.
 """
 
+import shutil
 import struct
 import pytest
 import u_boot_utils as util
 import vboot_forge
+import vboot_evil
 
+# Only run the full suite on a few combinations, since it doesn't add any more
+# test coverage.
 TESTDATA = [
-    ['sha1', '', None, False],
-    ['sha1', '', '-E -p 0x10000', False],
-    ['sha1', '-pss', None, False],
-    ['sha1', '-pss', '-E -p 0x10000', False],
-    ['sha256', '', None, False],
-    ['sha256', '', '-E -p 0x10000', False],
-    ['sha256', '-pss', None, False],
-    ['sha256', '-pss', '-E -p 0x10000', False],
-    ['sha256', '-pss', None, True],
-    ['sha256', '-pss', '-E -p 0x10000', True],
+    ['sha1', '', None, False, True],
+    ['sha1', '', '-E -p 0x10000', False, False],
+    ['sha1', '-pss', None, False, False],
+    ['sha1', '-pss', '-E -p 0x10000', False, False],
+    ['sha256', '', None, False, False],
+    ['sha256', '', '-E -p 0x10000', False, False],
+    ['sha256', '-pss', None, False, False],
+    ['sha256', '-pss', '-E -p 0x10000', False, False],
+    ['sha256', '-pss', None, True, False],
+    ['sha256', '-pss', '-E -p 0x10000', True, True],
 ]
 
 @pytest.mark.boardspec('sandbox')
@@ -48,8 +52,10 @@ TESTDATA = [
 @pytest.mark.requiredtool('fdtget')
 @pytest.mark.requiredtool('fdtput')
 @pytest.mark.requiredtool('openssl')
- at pytest.mark.parametrize("sha_algo,padding,sign_options,required", TESTDATA)
-def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
+ at pytest.mark.parametrize("sha_algo,padding,sign_options,required,full_test",
+                         TESTDATA)
+def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
+               full_test):
     """Test verified boot signing with mkimage and verification with 'bootm'.
 
     This works using sandbox only as it needs to update the device tree used
@@ -71,7 +77,7 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
         util.run_and_log(cons, 'dtc %s %s%s -O dtb '
                          '-o %s%s' % (dtc_args, datadir, dts, tmpdir, dtb))
 
-    def run_bootm(sha_algo, test_type, expect_string, boots):
+    def run_bootm(sha_algo, test_type, expect_string, boots, fit=None):
         """Run a 'bootm' command U-Boot.
 
         This always starts a fresh U-Boot instance since the device tree may
@@ -84,11 +90,14 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
                     use.
             boots: A boolean that is True if Linux should boot and False if
                     we are expected to not boot
+            fit: FIT filename to load and verify
         """
+        if not fit:
+            fit = '%stest.fit' % tmpdir
         cons.restart_uboot()
         with cons.log.section('Verified boot %s %s' % (sha_algo, test_type)):
             output = cons.run_command_list(
-                ['host load hostfs - 100 %stest.fit' % tmpdir,
+                ['host load hostfs - 100 %s' % fit,
                  'fdt addr 100',
                  'bootm 100'])
         assert expect_string in ''.join(output)
@@ -222,18 +231,41 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
 
         util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
 
-        # Make sure that U-Boot checks that the config is in the list of hashed
-        # nodes. If it isn't, a security bypass is possible.
-        with open(fit, 'rb') as fd:
-            root, strblock = vboot_forge.read_fdt(fd)
-        root, strblock = vboot_forge.manipulate(root, strblock)
-        with open(fit, 'w+b') as fd:
-            vboot_forge.write_fdt(root, strblock, fd)
-        util.run_and_log_expect_exception(
-            cons, [fit_check_sign, '-f', fit, '-k', dtb],
-            1, 'Failed to verify required signature')
-
-        run_bootm(sha_algo, 'forged config', 'Bad Data Hash', False)
+        if full_test:
+            # Make sure that U-Boot checks that the config is in the list of hashed
+            # nodes. If it isn't, a security bypass is possible.
+            ffit = '%stest.forged.fit' % tmpdir
+            shutil.copyfile(fit, ffit)
+            with open(ffit, 'rb') as fd:
+                root, strblock = vboot_forge.read_fdt(fd)
+            root, strblock = vboot_forge.manipulate(root, strblock)
+            with open(ffit, 'w+b') as fd:
+                vboot_forge.write_fdt(root, strblock, fd)
+            util.run_and_log_expect_exception(
+                cons, [fit_check_sign, '-f', ffit, '-k', dtb],
+                1, 'Failed to verify required signature')
+
+            run_bootm(sha_algo, 'forged config', 'Bad Data Hash', False, ffit)
+
+            # Try adding an evil root node. This should be detected.
+            efit = '%stest.evilf.fit' % tmpdir
+            shutil.copyfile(fit, efit)
+            vboot_evil.add_evil_node(fit, efit, evil_kernel, 'fakeroot')
+
+            util.run_and_log_expect_exception(
+                cons, [fit_check_sign, '-f', efit, '-k', dtb],
+                1, 'Failed to verify required signature')
+            run_bootm(sha_algo, 'evil fakeroot', 'Bad Data Hash', False, efit)
+
+            # Try adding an @ to the kernel node name. This should be detected.
+            efit = '%stest.evilk.fit' % tmpdir
+            shutil.copyfile(fit, efit)
+            vboot_evil.add_evil_node(fit, efit, evil_kernel, 'kernel@')
+
+            util.run_and_log_expect_exception(
+                cons, [fit_check_sign, '-f', efit, '-k', dtb],
+                1, 'Node name contains @')
+            run_bootm(sha_algo, 'evil kernel@', 'Bad Data Hash', False, efit)
 
         # Create a new properly signed fit and replace header bytes
         make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
@@ -344,8 +376,13 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required):
     create_rsa_pair('prod')
 
     # Create a number kernel image with zeroes
-    with open('%stest-kernel.bin' % tmpdir, 'w') as fd:
-        fd.write(500 * chr(0))
+    with open('%stest-kernel.bin' % tmpdir, 'wb') as fd:
+        fd.write(500 * b'\0')
+
+    # Create a second kernel image with ones
+    evil_kernel = '%stest-kernel1.bin' % tmpdir
+    with open(evil_kernel, 'wb') as fd:
+        fd.write(500 * b'\x01')
 
     try:
         # We need to use our own device tree file. Remember to restore it
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 5/8] image: Adjust the workings of fit_check_format()
  2021-02-16  0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
                   ` (3 preceding siblings ...)
  2021-02-16  0:08 ` [PATCH 4/8] test: Add tests for the 'evil' vboot attacks Simon Glass
@ 2021-02-16  0:08 ` Simon Glass
  2021-02-16  3:36   ` Tom Rini
  2021-02-17 13:30   ` 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
                   ` (2 subsequent siblings)
  7 siblings, 2 replies; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

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

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 6/8] image: Add an option to do a full check of the FIT
  2021-02-16  0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
                   ` (4 preceding siblings ...)
  2021-02-16  0:08 ` [PATCH 5/8] image: Adjust the workings of fit_check_format() Simon Glass
@ 2021-02-16  0:08 ` 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  0:08 ` [PATCH 8/8] image: Check for unit addresses in FITs Simon Glass
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

Some strange modifications of the FIT can introduce security risks. Add an
option to check it thoroughly, using libfdt's fdt_check_full() function.

Enable this by default if signature verification is enabled.

CVE-2021-27097

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>
---

 common/Kconfig.boot | 20 ++++++++++++++++++++
 common/image-fit.c  | 16 ++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/common/Kconfig.boot b/common/Kconfig.boot
index 5eaabdfc27f..7532e55edb8 100644
--- a/common/Kconfig.boot
+++ b/common/Kconfig.boot
@@ -63,6 +63,15 @@ config FIT_ENABLE_SHA512_SUPPORT
 	  SHA512 checksum is a 512-bit (64-byte) hash value used to check that
 	  the image contents have not been corrupted.
 
+config FIT_FULL_CHECK
+	bool "Do a full check of the FIT before using it"
+	default y
+	help
+	  Enable this do a full check of the FIT to make sure it is valid. This
+	  helps to protect against carefully crafted FITs which take advantage
+	  of bugs or omissions in the code. This includes a bad structure,
+	  multiple root nodes and the like.
+
 config FIT_SIGNATURE
 	bool "Enable signature verification of FIT uImages"
 	depends on DM
@@ -70,6 +79,7 @@ config FIT_SIGNATURE
 	select RSA
 	select RSA_VERIFY
 	select IMAGE_SIGN_INFO
+	select FIT_FULL_CHECK
 	help
 	  This option enables signature verification of FIT uImages,
 	  using a hash signed and verified using RSA. If
@@ -159,6 +169,15 @@ config SPL_FIT_PRINT
 	help
 	  Support printing the content of the fitImage in a verbose manner in SPL.
 
+config SPL_FIT_FULL_CHECK
+	bool "Do a full check of the FIT before using it"
+	help
+	  Enable this do a full check of the FIT to make sure it is valid. This
+	  helps to protect against carefully crafted FITs which take advantage
+	  of bugs or omissions in the code. This includes a bad structure,
+	  multiple root nodes and the like.
+
+
 config SPL_FIT_SIGNATURE
 	bool "Enable signature verification of FIT firmware within SPL"
 	depends on SPL_DM
@@ -168,6 +187,7 @@ config SPL_FIT_SIGNATURE
 	select SPL_RSA
 	select SPL_RSA_VERIFY
 	select SPL_IMAGE_SIGN_INFO
+	select SPL_FIT_FULL_CHECK
 
 config SPL_LOAD_FIT
 	bool "Enable SPL loading U-Boot as a FIT (basic fitImage features)"
diff --git a/common/image-fit.c b/common/image-fit.c
index f6c0428a96b..bcf395f6a18 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1580,6 +1580,22 @@ int fit_check_format(const void *fit, ulong size)
 		return -ENOEXEC;
 	}
 
+	if (CONFIG_IS_ENABLED(FIT_FULL_CHECK)) {
+		/*
+		 * If we are not given the size, make do wtih calculating it.
+		 * This is not as secure, so we should consider a flag to
+		 * control this.
+		 */
+		if (size == IMAGE_SIZE_INVAL)
+			size = fdt_totalsize(fit);
+		ret = fdt_check_full(fit, size);
+
+		if (ret) {
+			log_debug("FIT check error %d\n", ret);
+			return -EINVAL;
+		}
+	}
+
 	/* mandatory / node 'description' property */
 	if (!fdt_getprop(fit, 0, FIT_DESC_PROP, NULL)) {
 		log_debug("Wrong FIT format: no description\n");
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 7/8] libfdt: Check for multiple/invalid root nodes
  2021-02-16  0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
                   ` (5 preceding siblings ...)
  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  0:08 ` 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
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

It is possible to construct a devicetree blob with multiple root nodes.
Update fdt_check_full() to check for this, along with a root node with an
invalid name.

CVE-2021-27097

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>
---

 scripts/dtc/libfdt/fdt_ro.c | 17 +++++++++++++++++
 test/py/tests/test_vboot.py |  3 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/libfdt/fdt_ro.c b/scripts/dtc/libfdt/fdt_ro.c
index d984bab036b..efe7efe9211 100644
--- a/scripts/dtc/libfdt/fdt_ro.c
+++ b/scripts/dtc/libfdt/fdt_ro.c
@@ -867,6 +867,7 @@ int fdt_check_full(const void *fdt, size_t bufsize)
 	unsigned depth = 0;
 	const void *prop;
 	const char *propname;
+	bool expect_end = false;
 
 	if (bufsize < FDT_V1_SIZE)
 		return -FDT_ERR_TRUNCATED;
@@ -887,6 +888,10 @@ int fdt_check_full(const void *fdt, size_t bufsize)
 		if (nextoffset < 0)
 			return nextoffset;
 
+		/* If we see two root nodes, something is wrong */
+		if (expect_end && tag != FDT_END)
+			return -FDT_ERR_BADLAYOUT;
+
 		switch (tag) {
 		case FDT_NOP:
 			break;
@@ -900,12 +905,24 @@ int fdt_check_full(const void *fdt, size_t bufsize)
 			depth++;
 			if (depth > INT_MAX)
 				return -FDT_ERR_BADSTRUCTURE;
+
+			/* The root node must have an empty name */
+			if (depth == 1) {
+				const char *name;
+				int len;
+
+				name = fdt_get_name(fdt, offset, &len);
+				if (*name || len)
+					return -FDT_ERR_BADLAYOUT;
+			}
 			break;
 
 		case FDT_END_NODE:
 			if (depth == 0)
 				return -FDT_ERR_BADSTRUCTURE;
 			depth--;
+			if (depth == 0)
+				expect_end = true;
 			break;
 
 		case FDT_PROP:
diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index 9cdc290b9fc..22e8fc10d83 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -255,7 +255,8 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
             util.run_and_log_expect_exception(
                 cons, [fit_check_sign, '-f', efit, '-k', dtb],
                 1, 'Failed to verify required signature')
-            run_bootm(sha_algo, 'evil fakeroot', 'Bad Data Hash', False, efit)
+            run_bootm(sha_algo, 'evil fakeroot', 'Bad FIT kernel image format',
+                      False, efit)
 
             # Try adding an @ to the kernel node name. This should be detected.
             efit = '%stest.evilk.fit' % tmpdir
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 8/8] image: Check for unit addresses in FITs
  2021-02-16  0:08 [PATCH 0/8] vboot: Correct vulnerabilities identified by Intel Simon Glass
                   ` (6 preceding siblings ...)
  2021-02-16  0:08 ` [PATCH 7/8] libfdt: Check for multiple/invalid root nodes Simon Glass
@ 2021-02-16  0:08 ` Simon Glass
  2021-02-16  3:36   ` Tom Rini
  7 siblings, 1 reply; 20+ messages in thread
From: Simon Glass @ 2021-02-16  0:08 UTC (permalink / raw)
  To: u-boot

Using unit addresses in a FIT is a security risk. Add a check for this
and disallow it.

CVE-2021-27138

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>
---

 common/image-fit.c          | 56 ++++++++++++++++++++++++++++++++++---
 test/py/tests/test_vboot.py |  9 +++---
 2 files changed, 57 insertions(+), 8 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index bcf395f6a18..28b3d2b1911 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -1568,6 +1568,34 @@ int fit_image_check_comp(const void *fit, int noffset, uint8_t comp)
 	return (comp == image_comp);
 }
 
+/**
+ * fdt_check_no_at() - Check for nodes whose names contain '@'
+ *
+ * This checks the parent node and all subnodes recursively
+ *
+ * @fit: FIT to check
+ * @parent: Parent node to check
+ * @return 0 if OK, -EADDRNOTAVAIL is a node has a name containing '@'
+ */
+static int fdt_check_no_at(const void *fit, int parent)
+{
+	const char *name;
+	int node;
+	int ret;
+
+	name = fdt_get_name(fit, parent, NULL);
+	if (!name || strchr(name, '@'))
+		return -EADDRNOTAVAIL;
+
+	fdt_for_each_subnode(node, fit, parent) {
+		ret = fdt_check_no_at(fit, node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int fit_check_format(const void *fit, ulong size)
 {
 	int ret;
@@ -1589,10 +1617,27 @@ int fit_check_format(const void *fit, ulong size)
 		if (size == IMAGE_SIZE_INVAL)
 			size = fdt_totalsize(fit);
 		ret = fdt_check_full(fit, size);
+		if (ret)
+			ret = -EINVAL;
+
+		/*
+		 * U-Boot stopped using unit addressed in 2017. Since libfdt
+		 * can match nodes ignoring any unit address, signature
+		 * verification can see the wrong node if one is inserted with
+		 * the same name as a valid node but with a unit address
+		 * attached. Protect against this by disallowing unit addresses.
+		 */
+		if (!ret && CONFIG_IS_ENABLED(FIT_SIGNATURE)) {
+			ret = fdt_check_no_at(fit, 0);
 
+			if (ret) {
+				log_debug("FIT check error %d\n", ret);
+				return ret;
+			}
+		}
 		if (ret) {
 			log_debug("FIT check error %d\n", ret);
-			return -EINVAL;
+			return ret;
 		}
 	}
 
@@ -1955,10 +2000,13 @@ 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, IMAGE_SIZE_INVAL)) {
-		printf("Bad FIT %s image format!\n", prop_name);
+	ret = fit_check_format(fit, IMAGE_SIZE_INVAL);
+	if (ret) {
+		printf("Bad FIT %s image format! (err=%d)\n", prop_name, ret);
+		if (CONFIG_IS_ENABLED(FIT_SIGNATURE) && ret == -EADDRNOTAVAIL)
+			printf("Signature checking prevents use of unit addresses (@) in nodes\n");
 		bootstage_error(bootstage_id + BOOTSTAGE_SUB_FORMAT);
-		return -ENOEXEC;
+		return ret;
 	}
 	bootstage_mark(bootstage_id + BOOTSTAGE_SUB_FORMAT_OK);
 	if (fit_uname) {
diff --git a/test/py/tests/test_vboot.py b/test/py/tests/test_vboot.py
index 22e8fc10d83..6dff6779d17 100644
--- a/test/py/tests/test_vboot.py
+++ b/test/py/tests/test_vboot.py
@@ -232,8 +232,8 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
         util.run_and_log(cons, [fit_check_sign, '-f', fit, '-k', dtb])
 
         if full_test:
-            # Make sure that U-Boot checks that the config is in the list of hashed
-            # nodes. If it isn't, a security bypass is possible.
+            # Make sure that U-Boot checks that the config is in the list of
+            # hashed nodes. If it isn't, a security bypass is possible.
             ffit = '%stest.forged.fit' % tmpdir
             shutil.copyfile(fit, ffit)
             with open(ffit, 'rb') as fd:
@@ -263,10 +263,11 @@ def test_vboot(u_boot_console, sha_algo, padding, sign_options, required,
             shutil.copyfile(fit, efit)
             vboot_evil.add_evil_node(fit, efit, evil_kernel, 'kernel@')
 
+            msg = 'Signature checking prevents use of unit addresses (@) in nodes'
             util.run_and_log_expect_exception(
                 cons, [fit_check_sign, '-f', efit, '-k', dtb],
-                1, 'Node name contains @')
-            run_bootm(sha_algo, 'evil kernel@', 'Bad Data Hash', False, efit)
+                1, msg)
+            run_bootm(sha_algo, 'evil kernel@', msg, False, efit)
 
         # Create a new properly signed fit and replace header bytes
         make_fit('sign-configs-%s%s.its' % (sha_algo, padding))
-- 
2.30.0.478.g8a0d178c01-goog

^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 1/8] fdt_region: Check for a single root node of the correct name
  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
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:35 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:05PM -0700, Simon Glass wrote:

> At present fdt_find_regions() assumes that the FIT is a valid devicetree.
> If the FIT has two root nodes this is currently not detected in this
> function, nor does libfdt's fdt_check_full() notice. Also it is possible
> for the root node to have a name even though it should not.
> 
> Add checks for these and return -FDT_ERR_BADSTRUCTURE if a problem is
> detected.
> 
> CVE-2021-27097
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/1cc70bb1/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 2/8] fit: Don't allow verification of images with @ nodes
  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
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:35 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:06PM -0700, Simon Glass wrote:

> When searching for a node called 'fred', any unit address appended to the
> name is ignored by libfdt, meaning that 'fred' can match 'fred at 1'. This
> means that we cannot be sure that the node originally intended is the one
> that is used.
> 
> Disallow use of nodes with unit addresses.
> 
> Update the forge test also, since it uses @ addresses.
> 
> CVE-2021-27138
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/4f4da731/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 3/8] test: Add vboot_evil implementation
  2021-02-16  0:08 ` [PATCH 3/8] test: Add vboot_evil implementation Simon Glass
@ 2021-02-16  3:36   ` Tom Rini
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:36 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:07PM -0700, Simon Glass wrote:

> Add a library which performs two different attacks on a FIT.
> 
> Signed-off-by: Julien Lenoir <julien.lenoir@intel.com>
> Signed-off-by: Bruce Monroe <bruce.monroe@intel.com>
> Signed-off-by: Arie Haenel <arie.haenel@intel.com>
> Signed-off-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/7f77c832/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 4/8] test: Add tests for the 'evil' vboot attacks
  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
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:36 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:08PM -0700, Simon Glass wrote:

> Add tests to check that these two attacks are mitigated by recent patches.
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/e1590de3/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 5/8] image: Adjust the workings of fit_check_format()
  2021-02-16  0:08 ` [PATCH 5/8] image: Adjust the workings of fit_check_format() Simon Glass
@ 2021-02-16  3:36   ` Tom Rini
  2021-02-17 13:30   ` Jesper Schmitz Mouridsen
  1 sibling, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:36 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:09PM -0700, 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@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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/9ff98505/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 6/8] image: Add an option to do a full check of the FIT
  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
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:36 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:10PM -0700, Simon Glass wrote:

> Some strange modifications of the FIT can introduce security risks. Add an
> option to check it thoroughly, using libfdt's fdt_check_full() function.
> 
> Enable this by default if signature verification is enabled.
> 
> CVE-2021-27097
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/9ebc1aa8/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 7/8] libfdt: Check for multiple/invalid root nodes
  2021-02-16  0:08 ` [PATCH 7/8] libfdt: Check for multiple/invalid root nodes Simon Glass
@ 2021-02-16  3:36   ` Tom Rini
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:36 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:11PM -0700, Simon Glass wrote:

> It is possible to construct a devicetree blob with multiple root nodes.
> Update fdt_check_full() to check for this, along with a root node with an
> invalid name.
> 
> CVE-2021-27097
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/629bcf55/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 8/8] image: Check for unit addresses in FITs
  2021-02-16  0:08 ` [PATCH 8/8] image: Check for unit addresses in FITs Simon Glass
@ 2021-02-16  3:36   ` Tom Rini
  0 siblings, 0 replies; 20+ messages in thread
From: Tom Rini @ 2021-02-16  3:36 UTC (permalink / raw)
  To: u-boot

On Mon, Feb 15, 2021 at 05:08:12PM -0700, Simon Glass wrote:

> Using unit addresses in a FIT is a security risk. Add a check for this
> and disallow it.
> 
> CVE-2021-27138
> 
> 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>

Applied to u-boot/master, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210215/0948fece/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 5/8] image: Adjust the workings of fit_check_format()
  2021-02-16  0:08 ` [PATCH 5/8] image: Adjust the workings of fit_check_format() Simon Glass
  2021-02-16  3:36   ` Tom Rini
@ 2021-02-17 13:30   ` Jesper Schmitz Mouridsen
  2021-02-17 13:43     ` Tom Rini
  1 sibling, 1 reply; 20+ messages in thread
From: Jesper Schmitz Mouridsen @ 2021-02-17 13:30 UTC (permalink / raw)
  To: u-boot

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@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 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;

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 5/8] image: Adjust the workings of fit_check_format()
  2021-02-17 13:30   ` Jesper Schmitz Mouridsen
@ 2021-02-17 13:43     ` Tom Rini
  2021-02-17 14:12       ` Jesper Schmitz Mouridsen
  0 siblings, 1 reply; 20+ messages in thread
From: Tom Rini @ 2021-02-17 13:43 UTC (permalink / raw)
  To: u-boot

On Wed, Feb 17, 2021 at 02:30:56PM +0100, Jesper Schmitz Mouridsen wrote:

> Hi
> 
> Can you avoid the use of ENODATA since it is not defined in FreeBSD's
> 
> errno.h?

I like that FreeBSD has EDOOFUS, but I don't see an obvious replacement
for ENODATA, can you suggest something please?  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210217/1bc71192/attachment.sig>

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 5/8] image: Adjust the workings of fit_check_format()
  2021-02-17 13:43     ` Tom Rini
@ 2021-02-17 14:12       ` Jesper Schmitz Mouridsen
  0 siblings, 0 replies; 20+ messages in thread
From: Jesper Schmitz Mouridsen @ 2021-02-17 14:12 UTC (permalink / raw)
  To: u-boot

On 17.02.2021 14.43, Tom Rini wrote:
> On Wed, Feb 17, 2021 at 02:30:56PM +0100, Jesper Schmitz Mouridsen wrote:
>
>> Hi
>>
>> Can you avoid the use of ENODATA since it is not defined in FreeBSD's
>>
>> errno.h?
> I like that FreeBSD has EDOOFUS, but I don't see an obvious replacement
> for ENODATA, can you suggest something please?  Thanks!
>
Perhaps EBADMSG?

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2021-02-17 14:12 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` [PATCH 5/8] image: Adjust the workings of fit_check_format() Simon Glass
2021-02-16  3:36   ` 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

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.