[PATCH v5 0/7] libfdt: Allow more control of code size

[PATCH v5 0/7] libfdt: Allow more control of code size

[Simon Glass]

Since v1.4.6 libfdt has gained a number of checks for incorrect
device-tree input and incorrect parameters. These are valuable and should
be enabled by default. Some are basic sanity checks and some are designed
to avoid security risks with carefully crafted device-tree input.

However the checks have added to code size to such an extent that many
U-Boot boards can no-longer build/boot in SPL. This has prevented recent
versions of libfdt from being used by U-Boot.

U-Boot SPL generally reads a device tree which has been set up by the
build system and is known to be correct (e.g. it may be cryptographically
signed by the build system). Therefore sanity checks in SPL should never
fail, and just contribute to longer run-time and larger code size.

During review of those patches[1] the code-size impact was discussed and
a possible solution was suggested.

This series adds a new ASSUME_MASK option, which allows for some control
over the checks used in libfdt. With all assumptions enabled, libfdt
assumes that the input data and parameters are all correct and that
internal errors cannot happen. This allows U-Boot SPL to continue to use
later versions of libfdt.

Within the code, preprocessor macros and an inline function are used which
resolve to true or false to control the use of checks in the code. This
seems better than using '#ifdefs' throughout the code. It also ensures
that all code is compiled regardless of which checks are enabled (useful
for build coverage).

This series reduces the size of libfdt by about 3KB on 64-bit x86
(about 6%) when all assumptions are enabled. Future work is planned to
reduce this further, but this is a good start. It enables U-Boot to move
to the latest libfdt.

Note: libfdt includes a number of assignments in conditional statements.
I have only changed these where necessary. It might be desirable to remove
them all, to reduce the output from checkpatch.pl.

Series available at https://github.com/sglass68/dtc/tree/small4

[1] https://www.spinics.net/lists/devicetree-compiler/msg02166.html

Changes in v5:
- Include just VALID_INPUT checks in this patch
- Rename _can_assume() to can_assume_()
- Split SANE into VALID_DTB and VALID_INPUT
- Split out VALID_DTB checks into a separate patch
- Update comment for ASSUME_PERFECT to mention invalid parameters

Changes in v4:
- Add a can_assume() macros and squash the inline functions into one
- Add fdt_header_size to version.lds
- Drop unnecessary FDT_ prefix
- Fix 'Incosistencies' typo
- Merge the 'friendly' and 'sane' checks into one
- Update and expand comments

Changes in v3:
- Add a new patch to de-inline fdt_header_size()
- Expand the comments about each check option
- Fix 'santiy' typo
- Instead of excluding fdt_check_full() put it in its own file
- Invert the checks to be called assumptions
- Move header-version code to fdt.c
- Move the comments on check options to the enum
- Rearrange code in terms of checks instead of files changed, to aid review
- Replace 'unsigned' with 'unsigned int'
- Update commit message a little
- Use hex for CHK_MASK

Changes in v2:
- Add a comment to fdt_find_add_string_()
- Add an fdt_ prefix to avoid namespace conflicts
- Correct inverted version checks in a few cases
- Drop optimised code path in fdt_nodename_eq_()
- Update to use new check functions
- Use fdt_chk_base() in fdt_blocks_misordered_()
- Use symbolic names for _check functions and drop leading underscores

Simon Glass (7):
  libfdt: De-inline fdt_header_size()
  Add a way to control the level of checks in the code
  libfdt: Add support for disabling dtb checks
  libfdt: Add support for disabling sanity checks
  libfdt: Add support for disabling rollback handling
  libfdt: Add support for disabling version checks
  libfdt: Allow exclusion of fdt_check_full()

 Makefile                 |   6 +-
 libfdt/Makefile.libfdt   |   2 +-
 libfdt/fdt.c             |  95 ++++++++++++++----------
 libfdt/fdt_check.c       |  74 +++++++++++++++++++
 libfdt/fdt_ro.c          | 153 +++++++++++++++------------------------
 libfdt/fdt_rw.c          |  31 ++++++--
 libfdt/fdt_sw.c          |  30 +++++---
 libfdt/libfdt.h          |   9 ++-
 libfdt/libfdt_internal.h | 103 +++++++++++++++++++++++++-
 libfdt/version.lds       |   1 +
 tests/testutils.c        |   1 -
 11 files changed, 347 insertions(+), 158 deletions(-)
 create mode 100644 libfdt/fdt_check.c

-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 1/7] libfdt: De-inline fdt_header_size()

[Simon Glass]

There does not seem to be a strong reason to inline this function. Also
we are about to add some extra code to it which will increase its size.

Move it into fdt.c and use a simple declaration in libfdt.h

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v5: None
Changes in v4:
- Add fdt_header_size to version.lds

Changes in v3:
- Add a new patch to de-inline fdt_header_size()

Changes in v2: None

 libfdt/fdt.c       | 5 +++++
 libfdt/libfdt.h    | 9 +++++----
 libfdt/version.lds | 1 +
 tests/testutils.c  | 1 -
 4 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index d6ce7c0..3e37a4b 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -70,6 +70,11 @@ size_t fdt_header_size_(uint32_t version)
 		return FDT_V17_SIZE;
 }
 
+size_t fdt_header_size(const void *fdt)
+{
+	return fdt_header_size_(fdt_version(fdt));
+}
+
 int fdt_check_header(const void *fdt)
 {
 	size_t hdrsize;
diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
index fc4c496..48f375c 100644
--- a/libfdt/libfdt.h
+++ b/libfdt/libfdt.h
@@ -266,11 +266,12 @@ fdt_set_hdr_(size_dt_struct);
  * fdt_header_size - return the size of the tree's header
  * @fdt: pointer to a flattened device tree
  */
+size_t fdt_header_size(const void *fdt);
+
+/**
+ * fdt_header_size_ - internal function which takes a version number
+ */
 size_t fdt_header_size_(uint32_t version);
-static inline size_t fdt_header_size(const void *fdt)
-{
-	return fdt_header_size_(fdt_version(fdt));
-}
 
 /**
  * fdt_check_header - sanity check a device tree header
diff --git a/libfdt/version.lds b/libfdt/version.lds
index ae32924..7ab85f1 100644
--- a/libfdt/version.lds
+++ b/libfdt/version.lds
@@ -20,6 +20,7 @@ LIBFDT_1.2 {
 		fdt_get_alias_namelen;
 		fdt_get_alias;
 		fdt_get_path;
+                fdt_header_size;
 		fdt_supernode_atdepth_offset;
 		fdt_node_depth;
 		fdt_parent_offset;
diff --git a/tests/testutils.c b/tests/testutils.c
index 5e494c5..53cb35f 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -261,7 +261,6 @@ void vg_prepare_blob(void *fdt, size_t bufsize)
 
 	VALGRIND_MAKE_MEM_UNDEFINED(blob, bufsize);
 	VALGRIND_MAKE_MEM_DEFINED(blob, FDT_V1_SIZE);
-	VALGRIND_MAKE_MEM_DEFINED(blob, fdt_header_size(fdt));
 
 	if (fdt_magic(fdt) == FDT_MAGIC) {
 		off_strings = fdt_off_dt_strings(fdt);
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 2/7] Add a way to control the level of checks in the code

[Simon Glass]

Add a new ASSUME_MASK option, which allows for some control over the
checks used in libfdt. With all assumptions enabled, libfdt assumes that
the input data and parameters are all correct and that internal errors
cannot happen.

By default no assumptions are made and all checks are enabled.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v5:
- Rename _can_assume() to can_assume_()
- Split SANE into VALID_DTB and VALID_INPUT
- Update comment for ASSUME_PERFECT to mention invalid parameters

Changes in v4:
- Add a can_assume() macros and squash the inline functions into one
- Drop unnecessary FDT_ prefix
- Fix 'Incosistencies' typo
- Merge the 'friendly' and 'sane' checks into one
- Update and expand comments

Changes in v3:
- Expand the comments about each check option
- Invert the checks to be called assumptions
- Move header-version code to fdt.c
- Move the comments on check options to the enum
- Use hex for CHK_MASK

Changes in v2:
- Add an fdt_ prefix to avoid namespace conflicts
- Use symbolic names for _check functions and drop leading underscores

 Makefile                 |  6 ++-
 libfdt/libfdt_internal.h | 96 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index e73576a..0dff03f 100644
--- a/Makefile
+++ b/Makefile
@@ -16,7 +16,11 @@ EXTRAVERSION =
 LOCAL_VERSION =
 CONFIG_LOCALVERSION =
 
-CPPFLAGS = -I libfdt -I .
+# Control the assumptions made (e.g. risking security issues) in the code.
+# See libfdt_internal.h for details
+ASSUME_MASK ?= 0
+
+CPPFLAGS = -I libfdt -I . -DFDT_ASSUME_MASK=$(ASSUME_MASK)
 WARNINGS = -Wall -Wpointer-arith -Wcast-qual -Wnested-externs \
 	-Wstrict-prototypes -Wmissing-prototypes -Wredundant-decls -Wshadow
 CFLAGS = -g -Os $(SHAREDLIB_CFLAGS) -Werror $(WARNINGS) $(EXTRA_CFLAGS)
diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
index 058c735..1e31d10 100644
--- a/libfdt/libfdt_internal.h
+++ b/libfdt/libfdt_internal.h
@@ -48,4 +48,100 @@ static inline struct fdt_reserve_entry *fdt_mem_rsv_w_(void *fdt, int n)
 
 #define FDT_SW_MAGIC		(~FDT_MAGIC)
 
+/**********************************************************************/
+/* Checking controls                                                  */
+/**********************************************************************/
+
+#ifndef FDT_ASSUME_MASK
+#define FDT_ASSUME_MASK 0
+#endif
+
+/*
+ * Defines assumptions which can be enabled. Each of these can be enabled
+ * individually. For maximum saftey, don't enable any assumptions!
+ *
+ * For minimal code size and no safety, use ASSUME_PERFECT at your own risk.
+ * You should have another method of validating the device tree, such as a
+ * signature or hash check before using libfdt.
+ *
+ * For situations where security is not a concern it may be safe to enable
+ * ASSUME_SANE.
+ */
+enum {
+	/*
+	 * This does essentially no checks. Only the latest device-tree
+	 * version is correctly handled. Inconsistencies or errors in the device
+	 * tree may cause undefined behaviour or crashes. Invalid parameters
+	 * passed to libfdt may do the same.
+	 *
+	 * If an error occurs when modifying the tree it may leave the tree in
+	 * an intermediate (but valid) state. As an example, adding a property
+	 * where there is insufficient space may result in the property name
+	 * being added to the string table even though the property itself is
+	 * not added to the struct section.
+	 *
+	 * Only use this if you have a fully validated device tree with
+	 * the latest supported version and wish to minimise code size.
+	 */
+	ASSUME_PERFECT		= 0xff,
+
+	/*
+	 * This assumes that the device tree is sane. i.e. header metadata
+	 * and basic hierarchy are correct.
+	 *
+	 * With this assumption enabled, normal device trees produced by libfdt
+	 * and the compiler should be handled safely. Malicious device trees and
+	 * complete garbage may cause libfdt to behave badly or crash.
+	 *
+	 * Note: Only checks that relate exclusively to the device tree itself
+	 * (not the parameters passed to libfdt) are disabled by this
+	 * assumption. This includes checking headers, tags and the like.
+	 */
+	ASSUME_VALID_DTB	= 1 << 0,
+
+	/*
+	 * This builds on ASSUME_VALID_DTB and further assumes that libfdt
+	 * functions are called with valid parameters, i.e. not trigger
+	 * FDT_ERR_BADOFFSET or offsets that are out of bounds. It disables any
+	 * extensive checking of parameters and the device tree, making various
+	 * assumptions about correctness.
+	 *
+	 * It doesn't make sense to enable this assumption unless
+	 * ASSUME_VALID_DTB is also enabled.
+	 */
+	ASSUME_VALID_INPUT	= 1 << 1,
+
+	/*
+	 * This disables checks for device-tree version and removes all code
+	 * which handles older versions.
+	 *
+	 * Only enable this if you know you have a device tree with the latest
+	 * version.
+	 */
+	ASSUME_LATEST		= 1 << 2,
+
+	/*
+	 * This assume that it is OK for a failed additional to the device tree
+	 * due to lack of space or some other problem can skip any rollback
+	 * steps (such as dropping the property name from the string table).
+	 * This is safe to enable in most circumstances, even though it may
+	 * leave the tree in a sub-optimal state.
+	 */
+	ASSUME_NO_ROLLBACK	= 1 << 3,
+};
+
+/**
+ * can_assume_() - check if a particular assumption is enabled
+ *
+ * @mask: Mask to check (ASSUME_...)
+ * @return true if that assumption is enabled, else false
+ */
+static inline bool can_assume_(int mask)
+{
+	return FDT_ASSUME_MASK & mask;
+}
+
+/** helper macros for checking assumptions */
+#define can_assume(_assume)	can_assume_(ASSUME_ ## _assume)
+
 #endif /* LIBFDT_INTERNAL_H */
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[Simon Glass]

Support ASSUME_VALID_DTB to disable some sanity checks

If we assume that the DTB itself is valid then we can skip some checks and
save code space. Add various conditions to handle this.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v5:
- Split out VALID_DTB checks into a separate patch

Changes in v4: None
Changes in v3: None
Changes in v2: None

 libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
 libfdt/fdt_ro.c          |  7 ++++--
 libfdt/fdt_rw.c          |  7 +++++-
 libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
 libfdt/libfdt_internal.h |  7 ++++--
 5 files changed, 64 insertions(+), 37 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 3e37a4b..03f2b7d 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
 
 	if (fdt_magic(fdt) != FDT_MAGIC)
 		return -FDT_ERR_BADMAGIC;
-	hdrsize = fdt_header_size(fdt);
 	if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
 	    || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
 		return -FDT_ERR_BADVERSION;
 	if (fdt_version(fdt) < fdt_last_comp_version(fdt))
 		return -FDT_ERR_BADVERSION;
+	hdrsize = fdt_header_size(fdt);
+	if (!can_assume(VALID_DTB)) {
 
-	if ((fdt_totalsize(fdt) < hdrsize)
-	    || (fdt_totalsize(fdt) > INT_MAX))
-		return -FDT_ERR_TRUNCATED;
-
-	/* Bounds check memrsv block */
-	if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
-		return -FDT_ERR_TRUNCATED;
+		if ((fdt_totalsize(fdt) < hdrsize)
+		    || (fdt_totalsize(fdt) > INT_MAX))
+			return -FDT_ERR_TRUNCATED;
 
-	/* Bounds check structure block */
-	if (fdt_version(fdt) < 17) {
+		/* Bounds check memrsv block */
 		if (!check_off_(hdrsize, fdt_totalsize(fdt),
-				fdt_off_dt_struct(fdt)))
+				fdt_off_mem_rsvmap(fdt)))
 			return -FDT_ERR_TRUNCATED;
-	} else {
+	}
+
+	if (!can_assume(VALID_DTB)) {
+		/* Bounds check structure block */
+		if (fdt_version(fdt) < 17) {
+			if (!check_off_(hdrsize, fdt_totalsize(fdt),
+					fdt_off_dt_struct(fdt)))
+				return -FDT_ERR_TRUNCATED;
+		} else {
+			if (!check_block_(hdrsize, fdt_totalsize(fdt),
+					  fdt_off_dt_struct(fdt),
+					  fdt_size_dt_struct(fdt)))
+				return -FDT_ERR_TRUNCATED;
+		}
+
+		/* Bounds check strings block */
 		if (!check_block_(hdrsize, fdt_totalsize(fdt),
-				  fdt_off_dt_struct(fdt),
-				  fdt_size_dt_struct(fdt)))
+				  fdt_off_dt_strings(fdt),
+				  fdt_size_dt_strings(fdt)))
 			return -FDT_ERR_TRUNCATED;
 	}
 
-	/* Bounds check strings block */
-	if (!check_block_(hdrsize, fdt_totalsize(fdt),
-			  fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
-		return -FDT_ERR_TRUNCATED;
-
 	return 0;
 }
 
@@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 
 	*nextoffset = -FDT_ERR_TRUNCATED;
 	tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
-	if (!tagp)
+	if (!can_assume(VALID_DTB) && !tagp)
 		return FDT_END; /* premature end */
 	tag = fdt32_to_cpu(*tagp);
 	offset += FDT_TAGSIZE;
@@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		do {
 			p = fdt_offset_ptr(fdt, offset++, 1);
 		} while (p && (*p != '\0'));
-		if (!p)
+		if (!can_assume(VALID_DTB) && !p)
 			return FDT_END; /* premature end */
 		break;
 
 	case FDT_PROP:
 		lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
-		if (!lenp)
+		if (!can_assume(VALID_DTB) && !lenp)
 			return FDT_END; /* premature end */
 		/* skip-name offset, length and value */
 		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index a5c2797..b41083f 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 	const char *nameptr;
 	int err;
 
-	if (((err = fdt_ro_probe_(fdt)) < 0)
-	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
+	if (!can_assume(VALID_DTB)) {
+		if ((err = fdt_ro_probe_(fdt)) < 0)
 			goto fail;
+		if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
+			goto fail;
+	}
 
 	nameptr = nh->name;
 
diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 8795947..7a41a31 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -13,6 +13,8 @@
 static int fdt_blocks_misordered_(const void *fdt,
 				  int mem_rsv_size, int struct_size)
 {
+	if (can_assume(VALID_DTB))
+		return false;
 	return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
 		|| (fdt_off_dt_struct(fdt) <
 		    (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
@@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
 
 static int fdt_rw_probe_(void *fdt)
 {
+	if (can_assume(VALID_DTB))
+		return 0;
 	FDT_RO_PROBE(fdt);
 
 	if (fdt_version(fdt) < 17)
@@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
 #define FDT_RW_PROBE(fdt) \
 	{ \
 		int err_; \
-		if ((err_ = fdt_rw_probe_(fdt)) != 0) \
+		if (!can_assume(VALID_DTB) && \
+		    (err_ = fdt_rw_probe_(fdt)) != 0) \
 			return err_; \
 	}
 
diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
index 76bea22..8fca816 100644
--- a/libfdt/fdt_sw.c
+++ b/libfdt/fdt_sw.c
@@ -12,17 +12,20 @@
 
 static int fdt_sw_probe_(void *fdt)
 {
-	if (fdt_magic(fdt) == FDT_MAGIC)
-		return -FDT_ERR_BADSTATE;
-	else if (fdt_magic(fdt) != FDT_SW_MAGIC)
-		return -FDT_ERR_BADMAGIC;
+	if (!can_assume(VALID_DTB)) {
+		if (fdt_magic(fdt) == FDT_MAGIC)
+			return -FDT_ERR_BADSTATE;
+		else if (fdt_magic(fdt) != FDT_SW_MAGIC)
+			return -FDT_ERR_BADMAGIC;
+	}
+
 	return 0;
 }
 
 #define FDT_SW_PROBE(fdt) \
 	{ \
 		int err; \
-		if ((err = fdt_sw_probe_(fdt)) != 0) \
+		if (!can_assume(VALID_DTB) && (err = fdt_sw_probe_(fdt)) != 0) \
 			return err; \
 	}
 
@@ -38,7 +41,7 @@ static int fdt_sw_probe_memrsv_(void *fdt)
 	if (err)
 		return err;
 
-	if (fdt_off_dt_strings(fdt) != 0)
+	if (!can_assume(VALID_DTB) && fdt_off_dt_strings(fdt) != 0)
 		return -FDT_ERR_BADSTATE;
 	return 0;
 }
@@ -46,7 +49,8 @@ static int fdt_sw_probe_memrsv_(void *fdt)
 #define FDT_SW_PROBE_MEMRSV(fdt) \
 	{ \
 		int err; \
-		if ((err = fdt_sw_probe_memrsv_(fdt)) != 0) \
+		if (!can_assume(VALID_DTB) && \
+		    (err = fdt_sw_probe_memrsv_(fdt)) != 0) \
 			return err; \
 	}
 
@@ -60,7 +64,11 @@ static int fdt_sw_probe_memrsv_(void *fdt)
  */
 static int fdt_sw_probe_struct_(void *fdt)
 {
-	int err = fdt_sw_probe_(fdt);
+	int err;
+
+	if (can_assume(VALID_DTB))
+		return 0;
+	err = fdt_sw_probe_(fdt);
 	if (err)
 		return err;
 
@@ -72,7 +80,8 @@ static int fdt_sw_probe_struct_(void *fdt)
 #define FDT_SW_PROBE_STRUCT(fdt) \
 	{ \
 		int err; \
-		if ((err = fdt_sw_probe_struct_(fdt)) != 0) \
+		if (!can_assume(VALID_DTB) && \
+		    (err = fdt_sw_probe_struct_(fdt)) != 0) \
 			return err; \
 	}
 
@@ -151,7 +160,8 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
 	headsize = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
 	tailsize = fdt_size_dt_strings(fdt);
 
-	if ((headsize + tailsize) > fdt_totalsize(fdt))
+	if (!can_assume(VALID_DTB) &&
+	    headsize + tailsize > fdt_totalsize(fdt))
 		return -FDT_ERR_INTERNAL;
 
 	if ((headsize + tailsize) > bufsize)
diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
index 1e31d10..1aa732b 100644
--- a/libfdt/libfdt_internal.h
+++ b/libfdt/libfdt_internal.h
@@ -14,8 +14,11 @@ int32_t fdt_ro_probe_(const void *fdt);
 #define FDT_RO_PROBE(fdt)					\
 	{							\
 		int32_t totalsize_;				\
-		if ((totalsize_ = fdt_ro_probe_(fdt)) < 0)	\
-			return totalsize_;			\
+		if (!can_assume(VALID_DTB)) {			\
+			totalsize_ = fdt_ro_probe_(fdt);	\
+			if (totalsize_ < 0)			\
+				return totalsize_;		\
+		}						\
 	}
 
 int fdt_check_node_offset_(const void *fdt, int offset);
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 4/7] libfdt: Add support for disabling sanity checks

[Simon Glass]

Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
tree and the parameters to libfdt. This assumption covers that cases where
the problem could be with either.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v5:
- Include just VALID_INPUT checks in this patch

Changes in v4: None
Changes in v3: None
Changes in v2: None

 libfdt/fdt.c    | 12 +++++----
 libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
 2 files changed, 54 insertions(+), 29 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index 03f2b7d..e2c1da0 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 {
 	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
 
-	if ((absoffset < offset)
-	    || ((absoffset + len) < absoffset)
-	    || (absoffset + len) > fdt_totalsize(fdt))
-		return NULL;
+	if (!can_assume(VALID_INPUT))
+		if ((absoffset < offset)
+		    || ((absoffset + len) < absoffset)
+		    || (absoffset + len) > fdt_totalsize(fdt))
+			return NULL;
 
 	if (fdt_version(fdt) >= 0x11)
 		if (((offset + len) < offset)
@@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		return FDT_END;
 	}
 
-	if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
+	if (!can_assume(VALID_INPUT) &&
+	    !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
 		return FDT_END; /* premature end */
 
 	*nextoffset = FDT_TAGALIGN(offset);
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index b41083f..07c13c9 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
 	int olen;
 	const char *p = fdt_get_name(fdt, offset, &olen);
 
-	if (!p || olen < len)
+	if (!p || (!can_assume(VALID_INPUT) && olen < len))
 		/* short match */
 		return 0;
 
@@ -33,17 +33,26 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
 
 const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 {
-	int32_t totalsize = fdt_ro_probe_(fdt);
-	uint32_t absoffset = stroffset + fdt_off_dt_strings(fdt);
+	int32_t totalsize;
+	uint32_t absoffset;
 	size_t len;
 	int err;
 	const char *s, *n;
 
+	if (can_assume(VALID_INPUT)) {
+		s = (const char *)fdt + fdt_off_dt_strings(fdt) + stroffset;
+
+		if (lenp)
+			*lenp = strlen(s);
+		return s;
+	}
+	totalsize = fdt_ro_probe_(fdt);
 	err = totalsize;
 	if (totalsize < 0)
 		goto fail;
 
 	err = -FDT_ERR_BADOFFSET;
+	absoffset = stroffset + fdt_off_dt_strings(fdt);
 	if (absoffset >= totalsize)
 		goto fail;
 	len = totalsize - absoffset;
@@ -151,10 +160,13 @@ static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
 	int offset = n * sizeof(struct fdt_reserve_entry);
 	int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
 
-	if (absoffset < fdt_off_mem_rsvmap(fdt))
-		return NULL;
-	if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
-		return NULL;
+	if (!can_assume(VALID_INPUT)) {
+		if (absoffset < fdt_off_mem_rsvmap(fdt))
+			return NULL;
+		if (absoffset > fdt_totalsize(fdt) -
+		    sizeof(struct fdt_reserve_entry))
+			return NULL;
+	}
 	return fdt_mem_rsv_(fdt, n);
 }
 
@@ -164,7 +176,7 @@ int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
 
 	FDT_RO_PROBE(fdt);
 	re = fdt_mem_rsv(fdt, n);
-	if (!re)
+	if (!can_assume(VALID_INPUT) && !re)
 		return -FDT_ERR_BADOFFSET;
 
 	*address = fdt64_ld(&re->address);
@@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 	if (!can_assume(VALID_DTB)) {
 		if ((err = fdt_ro_probe_(fdt)) < 0)
 			goto fail;
-		if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
-			goto fail;
+		if (can_assume(VALID_INPUT) &&
+		    (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
+		goto fail;
 	}
 
 	nameptr = nh->name;
@@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
 	int err;
 	const struct fdt_property *prop;
 
-	if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
+	if (!can_assume(VALID_INPUT) &&
+	    (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
 		if (lenp)
 			*lenp = err;
 		return NULL;
@@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
 	     (offset = fdt_next_property_offset(fdt, offset))) {
 		const struct fdt_property *prop;
 
-		if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
+		prop = fdt_get_property_by_offset_(fdt, offset, lenp);
+		if (!can_assume(VALID_INPUT) && !prop) {
 			offset = -FDT_ERR_INTERNAL;
 			break;
 		}
@@ -464,14 +479,19 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 	if (namep) {
 		const char *name;
 		int namelen;
-		name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
-				      &namelen);
-		if (!name) {
-			if (lenp)
-				*lenp = namelen;
-			return NULL;
+
+		if (!can_assume(VALID_INPUT)) {
+			name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
+					      &namelen);
+			if (!name) {
+				if (lenp)
+					*lenp = namelen;
+				return NULL;
+			}
+			*namep = name;
+		} else {
+			*namep = fdt_string(fdt, fdt32_ld(&prop->nameoff));
 		}
-		*namep = name;
 	}
 
 	/* Handle realignment */
@@ -601,10 +621,12 @@ int fdt_supernode_atdepth_offset(const void *fdt, int nodeoffset,
 		}
 	}
 
-	if ((offset == -FDT_ERR_NOTFOUND) || (offset >= 0))
-		return -FDT_ERR_BADOFFSET;
-	else if (offset == -FDT_ERR_BADOFFSET)
-		return -FDT_ERR_BADSTRUCTURE;
+	if (!can_assume(VALID_INPUT)) {
+		if ((offset == -FDT_ERR_NOTFOUND) || (offset >= 0))
+			return -FDT_ERR_BADOFFSET;
+		else if (offset == -FDT_ERR_BADOFFSET)
+			return -FDT_ERR_BADSTRUCTURE;
+	}
 
 	return offset; /* error from fdt_next_node() */
 }
@@ -616,7 +638,8 @@ int fdt_node_depth(const void *fdt, int nodeoffset)
 
 	err = fdt_supernode_atdepth_offset(fdt, nodeoffset, 0, &nodedepth);
 	if (err)
-		return (err < 0) ? err : -FDT_ERR_INTERNAL;
+		return (can_assume(VALID_INPUT) || err < 0) ? err :
+			-FDT_ERR_INTERNAL;
 	return nodedepth;
 }
 
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 5/7] libfdt: Add support for disabling rollback handling

[Simon Glass]

Allow enabling FDT_ASSUME_NO_ROLLBACK to disable rolling back after a
failed operation.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 libfdt/fdt_rw.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 7a41a31..75f70d9 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -117,6 +117,15 @@ static int fdt_splice_string_(void *fdt, int newlen)
 	return 0;
 }
 
+/**
+ * fdt_find_add_string_() - Find or allocate a string
+ *
+ * @fdt: pointer to the device tree to check/adjust
+ * @s: string to find/add
+ * @allocated: Set to 0 if the string was found, 1 if not found and so
+ *	allocated. Ignored if can_assume(NO_ROLLBACK)
+ * @return offset of string in the string table (whether found or added)
+ */
 static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
 {
 	char *strtab = (char *)fdt + fdt_off_dt_strings(fdt);
@@ -125,7 +134,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
 	int len = strlen(s) + 1;
 	int err;
 
-	*allocated = 0;
+	if (!can_assume(NO_ROLLBACK))
+		*allocated = 0;
 
 	p = fdt_find_string_(strtab, fdt_size_dt_strings(fdt), s);
 	if (p)
@@ -137,7 +147,8 @@ static int fdt_find_add_string_(void *fdt, const char *s, int *allocated)
 	if (err)
 		return err;
 
-	*allocated = 1;
+	if (!can_assume(NO_ROLLBACK))
+		*allocated = 1;
 
 	memcpy(new, s, len);
 	return (new - strtab);
@@ -211,7 +222,8 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name,
 
 	err = fdt_splice_struct_(fdt, *prop, 0, proplen);
 	if (err) {
-		if (allocated)
+		/* Delete the string if we failed to add it */
+		if (!can_assume(NO_ROLLBACK) && allocated)
 			fdt_del_last_string_(fdt, name);
 		return err;
 	}
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 6/7] libfdt: Add support for disabling version checks

[Simon Glass]

Allow enabling FDT_ASSUME_LATEST to disable version checks.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None

 libfdt/fdt.c    | 34 +++++++++++++++++++++-------------
 libfdt/fdt_ro.c | 16 ++++++++--------
 libfdt/fdt_rw.c |  6 +++---
 3 files changed, 32 insertions(+), 24 deletions(-)

diff --git a/libfdt/fdt.c b/libfdt/fdt.c
index e2c1da0..0ac8736 100644
--- a/libfdt/fdt.c
+++ b/libfdt/fdt.c
@@ -21,10 +21,13 @@ int32_t fdt_ro_probe_(const void *fdt)
 
 	if (fdt_magic(fdt) == FDT_MAGIC) {
 		/* Complete tree */
-		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
-			return -FDT_ERR_BADVERSION;
-		if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
-			return -FDT_ERR_BADVERSION;
+		if (!can_assume(LATEST)) {
+			if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
+				return -FDT_ERR_BADVERSION;
+			if (fdt_last_comp_version(fdt) >
+					FDT_LAST_SUPPORTED_VERSION)
+				return -FDT_ERR_BADVERSION;
+		}
 	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
 		/* Unfinished sequential-write blob */
 		if (fdt_size_dt_struct(fdt) == 0)
@@ -72,7 +75,8 @@ size_t fdt_header_size_(uint32_t version)
 
 size_t fdt_header_size(const void *fdt)
 {
-	return fdt_header_size_(fdt_version(fdt));
+	return can_assume(LATEST) ? FDT_V17_SIZE :
+		fdt_header_size_(fdt_version(fdt));
 }
 
 int fdt_check_header(const void *fdt)
@@ -81,11 +85,14 @@ int fdt_check_header(const void *fdt)
 
 	if (fdt_magic(fdt) != FDT_MAGIC)
 		return -FDT_ERR_BADMAGIC;
-	if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
-	    || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
-		return -FDT_ERR_BADVERSION;
-	if (fdt_version(fdt) < fdt_last_comp_version(fdt))
-		return -FDT_ERR_BADVERSION;
+	if (!can_assume(LATEST)) {
+		if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
+		    || (fdt_last_comp_version(fdt) >
+			FDT_LAST_SUPPORTED_VERSION))
+			return -FDT_ERR_BADVERSION;
+		if (fdt_version(fdt) < fdt_last_comp_version(fdt))
+			return -FDT_ERR_BADVERSION;
+	}
 	hdrsize = fdt_header_size(fdt);
 	if (!can_assume(VALID_DTB)) {
 
@@ -101,7 +108,7 @@ int fdt_check_header(const void *fdt)
 
 	if (!can_assume(VALID_DTB)) {
 		/* Bounds check structure block */
-		if (fdt_version(fdt) < 17) {
+		if (!can_assume(LATEST) && fdt_version(fdt) < 17) {
 			if (!check_off_(hdrsize, fdt_totalsize(fdt),
 					fdt_off_dt_struct(fdt)))
 				return -FDT_ERR_TRUNCATED;
@@ -132,7 +139,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
 		    || (absoffset + len) > fdt_totalsize(fdt))
 			return NULL;
 
-	if (fdt_version(fdt) >= 0x11)
+	if (can_assume(LATEST) || fdt_version(fdt) >= 0x11)
 		if (((offset + len) < offset)
 		    || ((offset + len) > fdt_size_dt_struct(fdt)))
 			return NULL;
@@ -172,7 +179,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
 		/* skip-name offset, length and value */
 		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
 			+ fdt32_to_cpu(*lenp);
-		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
+		if (!can_assume(LATEST) &&
+		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
 		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
 			offset += 4;
 		break;
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 07c13c9..63cb60e 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -60,7 +60,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
 	if (fdt_magic(fdt) == FDT_MAGIC) {
 		if (stroffset < 0)
 			goto fail;
-		if (fdt_version(fdt) >= 17) {
+		if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
 			if (stroffset >= fdt_size_dt_strings(fdt))
 				goto fail;
 			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
@@ -311,7 +311,7 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
 
 	nameptr = nh->name;
 
-	if (fdt_version(fdt) < 0x10) {
+	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
 		/*
 		 * For old FDT versions, match the naming conventions of V16:
 		 * give only the leaf name (after all /). The actual tree
@@ -384,7 +384,7 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
 	/* Prior to version 16, properties may need realignment
 	 * and this API does not work. fdt_getprop_*() will, however. */
 
-	if (fdt_version(fdt) < 0x10) {
+	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
 		if (lenp)
 			*lenp = -FDT_ERR_BADVERSION;
 		return NULL;
@@ -431,7 +431,7 @@ const struct fdt_property *fdt_get_property_namelen(const void *fdt,
 {
 	/* Prior to version 16, properties may need realignment
 	 * and this API does not work. fdt_getprop_*() will, however. */
-	if (fdt_version(fdt) < 0x10) {
+	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
 		if (lenp)
 			*lenp = -FDT_ERR_BADVERSION;
 		return NULL;
@@ -462,8 +462,8 @@ const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
 		return NULL;
 
 	/* Handle realignment */
-	if (fdt_version(fdt) < 0x10 && (poffset + sizeof(*prop)) % 8 &&
-	    fdt32_ld(&prop->len) >= 8)
+	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
+	    (poffset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
 		return prop->data + 4;
 	return prop->data;
 }
@@ -495,8 +495,8 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
 	}
 
 	/* Handle realignment */
-	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
-	    fdt32_ld(&prop->len) >= 8)
+	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
+	    (offset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
 		return prop->data + 4;
 	return prop->data;
 }
diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
index 75f70d9..a18e123 100644
--- a/libfdt/fdt_rw.c
+++ b/libfdt/fdt_rw.c
@@ -30,12 +30,12 @@ static int fdt_rw_probe_(void *fdt)
 		return 0;
 	FDT_RO_PROBE(fdt);
 
-	if (fdt_version(fdt) < 17)
+	if (!can_assume(LATEST) && fdt_version(fdt) < 17)
 		return -FDT_ERR_BADVERSION;
 	if (fdt_blocks_misordered_(fdt, sizeof(struct fdt_reserve_entry),
 				   fdt_size_dt_struct(fdt)))
 		return -FDT_ERR_BADLAYOUT;
-	if (fdt_version(fdt) > 17)
+	if (!can_assume(LATEST) && fdt_version(fdt) > 17)
 		fdt_set_version(fdt, 17);
 
 	return 0;
@@ -428,7 +428,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
 	mem_rsv_size = (fdt_num_mem_rsv(fdt)+1)
 		* sizeof(struct fdt_reserve_entry);
 
-	if (fdt_version(fdt) >= 17) {
+	if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
 		struct_size = fdt_size_dt_struct(fdt);
 	} else {
 		struct_size = 0;
-- 
2.25.0.341.g760bfbb309-goog

[PATCH v5 7/7] libfdt: Allow exclusion of fdt_check_full()

[Simon Glass]

This function is used to perform a full check of the device tree. Allow
it to be excluded if all assumptions are enabled.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>
---

Changes in v5: None
Changes in v4: None
Changes in v3:
- Fix 'santiy' typo
- Instead of excluding fdt_check_full() put it in its own file
- Rearrange code in terms of checks instead of files changed, to aid review
- Replace 'unsigned' with 'unsigned int'
- Update commit message a little

Changes in v2:
- Add a comment to fdt_find_add_string_()
- Correct inverted version checks in a few cases
- Drop optimised code path in fdt_nodename_eq_()
- Update to use new check functions
- Use fdt_chk_base() in fdt_blocks_misordered_()

 libfdt/Makefile.libfdt |  2 +-
 libfdt/fdt_check.c     | 74 ++++++++++++++++++++++++++++++++++++++++++
 libfdt/fdt_ro.c        | 63 -----------------------------------
 3 files changed, 75 insertions(+), 64 deletions(-)
 create mode 100644 libfdt/fdt_check.c

diff --git a/libfdt/Makefile.libfdt b/libfdt/Makefile.libfdt
index e546397..b6d8fc0 100644
--- a/libfdt/Makefile.libfdt
+++ b/libfdt/Makefile.libfdt
@@ -8,7 +8,7 @@ LIBFDT_soname = libfdt.$(SHAREDLIB_EXT).1
 LIBFDT_INCLUDES = fdt.h libfdt.h libfdt_env.h
 LIBFDT_VERSION = version.lds
 LIBFDT_SRCS = fdt.c fdt_ro.c fdt_wip.c fdt_sw.c fdt_rw.c fdt_strerror.c fdt_empty_tree.c \
-	fdt_addresses.c fdt_overlay.c
+	fdt_addresses.c fdt_overlay.c fdt_check.c
 LIBFDT_OBJS = $(LIBFDT_SRCS:%.c=%.o)
 LIBFDT_LIB = libfdt-$(DTC_VERSION).$(SHAREDLIB_EXT)
 
diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c
new file mode 100644
index 0000000..7f6a96c
--- /dev/null
+++ b/libfdt/fdt_check.c
@@ -0,0 +1,74 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR BSD-2-Clause)
+/*
+ * libfdt - Flat Device Tree manipulation
+ * Copyright (C) 2006 David Gibson, IBM Corporation.
+ */
+#include "libfdt_env.h"
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "libfdt_internal.h"
+
+int fdt_check_full(const void *fdt, size_t bufsize)
+{
+	int err;
+	int num_memrsv;
+	int offset, nextoffset = 0;
+	uint32_t tag;
+	unsigned int depth = 0;
+	const void *prop;
+	const char *propname;
+
+	if (bufsize < FDT_V1_SIZE)
+		return -FDT_ERR_TRUNCATED;
+	err = fdt_check_header(fdt);
+	if (err != 0)
+		return err;
+	if (bufsize < fdt_totalsize(fdt))
+		return -FDT_ERR_TRUNCATED;
+
+	num_memrsv = fdt_num_mem_rsv(fdt);
+	if (num_memrsv < 0)
+		return num_memrsv;
+
+	while (1) {
+		offset = nextoffset;
+		tag = fdt_next_tag(fdt, offset, &nextoffset);
+
+		if (nextoffset < 0)
+			return nextoffset;
+
+		switch (tag) {
+		case FDT_NOP:
+			break;
+
+		case FDT_END:
+			if (depth != 0)
+				return -FDT_ERR_BADSTRUCTURE;
+			return 0;
+
+		case FDT_BEGIN_NODE:
+			depth++;
+			if (depth > INT_MAX)
+				return -FDT_ERR_BADSTRUCTURE;
+			break;
+
+		case FDT_END_NODE:
+			if (depth == 0)
+				return -FDT_ERR_BADSTRUCTURE;
+			depth--;
+			break;
+
+		case FDT_PROP:
+			prop = fdt_getprop_by_offset(fdt, offset, &propname,
+						     &err);
+			if (!prop)
+				return err;
+			break;
+
+		default:
+			return -FDT_ERR_INTERNAL;
+		}
+	}
+}
diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index 63cb60e..c29925d 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -859,66 +859,3 @@ int fdt_node_offset_by_compatible(const void *fdt, int startoffset,
 
 	return offset; /* error from fdt_next_node() */
 }
-
-int fdt_check_full(const void *fdt, size_t bufsize)
-{
-	int err;
-	int num_memrsv;
-	int offset, nextoffset = 0;
-	uint32_t tag;
-	unsigned depth = 0;
-	const void *prop;
-	const char *propname;
-
-	if (bufsize < FDT_V1_SIZE)
-		return -FDT_ERR_TRUNCATED;
-	err = fdt_check_header(fdt);
-	if (err != 0)
-		return err;
-	if (bufsize < fdt_totalsize(fdt))
-		return -FDT_ERR_TRUNCATED;
-
-	num_memrsv = fdt_num_mem_rsv(fdt);
-	if (num_memrsv < 0)
-		return num_memrsv;
-
-	while (1) {
-		offset = nextoffset;
-		tag = fdt_next_tag(fdt, offset, &nextoffset);
-
-		if (nextoffset < 0)
-			return nextoffset;
-
-		switch (tag) {
-		case FDT_NOP:
-			break;
-
-		case FDT_END:
-			if (depth != 0)
-				return -FDT_ERR_BADSTRUCTURE;
-			return 0;
-
-		case FDT_BEGIN_NODE:
-			depth++;
-			if (depth > INT_MAX)
-				return -FDT_ERR_BADSTRUCTURE;
-			break;
-
-		case FDT_END_NODE:
-			if (depth == 0)
-				return -FDT_ERR_BADSTRUCTURE;
-			depth--;
-			break;
-
-		case FDT_PROP:
-			prop = fdt_getprop_by_offset(fdt, offset, &propname,
-						     &err);
-			if (!prop)
-				return err;
-			break;
-
-		default:
-			return -FDT_ERR_INTERNAL;
-		}
-	}
-}
-- 
2.25.0.341.g760bfbb309-goog

Re: [PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9142 bytes --]

On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> Support ASSUME_VALID_DTB to disable some sanity checks
> 
> If we assume that the DTB itself is valid then we can skip some checks and
> save code space. Add various conditions to handle this.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v5:
> - Split out VALID_DTB checks into a separate patch
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
>  libfdt/fdt_ro.c          |  7 ++++--
>  libfdt/fdt_rw.c          |  7 +++++-
>  libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
>  libfdt/libfdt_internal.h |  7 ++++--
>  5 files changed, 64 insertions(+), 37 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 3e37a4b..03f2b7d 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
>  
>  	if (fdt_magic(fdt) != FDT_MAGIC)
>  		return -FDT_ERR_BADMAGIC;
> -	hdrsize = fdt_header_size(fdt);
>  	if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
>  	    || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
>  		return -FDT_ERR_BADVERSION;
>  	if (fdt_version(fdt) < fdt_last_comp_version(fdt))
>  		return -FDT_ERR_BADVERSION;
> +	hdrsize = fdt_header_size(fdt);
> +	if (!can_assume(VALID_DTB)) {
>  
> -	if ((fdt_totalsize(fdt) < hdrsize)
> -	    || (fdt_totalsize(fdt) > INT_MAX))
> -		return -FDT_ERR_TRUNCATED;
> -
> -	/* Bounds check memrsv block */
> -	if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> -		return -FDT_ERR_TRUNCATED;
> +		if ((fdt_totalsize(fdt) < hdrsize)
> +		    || (fdt_totalsize(fdt) > INT_MAX))
> +			return -FDT_ERR_TRUNCATED;
>  
> -	/* Bounds check structure block */
> -	if (fdt_version(fdt) < 17) {
> +		/* Bounds check memrsv block */
>  		if (!check_off_(hdrsize, fdt_totalsize(fdt),
> -				fdt_off_dt_struct(fdt)))
> +				fdt_off_mem_rsvmap(fdt)))
>  			return -FDT_ERR_TRUNCATED;
> -	} else {
> +	}
> +
> +	if (!can_assume(VALID_DTB)) {
> +		/* Bounds check structure block */
> +		if (fdt_version(fdt) < 17) {
> +			if (!check_off_(hdrsize, fdt_totalsize(fdt),
> +					fdt_off_dt_struct(fdt)))
> +				return -FDT_ERR_TRUNCATED;
> +		} else {
> +			if (!check_block_(hdrsize, fdt_totalsize(fdt),
> +					  fdt_off_dt_struct(fdt),
> +					  fdt_size_dt_struct(fdt)))
> +				return -FDT_ERR_TRUNCATED;
> +		}
> +
> +		/* Bounds check strings block */
>  		if (!check_block_(hdrsize, fdt_totalsize(fdt),
> -				  fdt_off_dt_struct(fdt),
> -				  fdt_size_dt_struct(fdt)))
> +				  fdt_off_dt_strings(fdt),
> +				  fdt_size_dt_strings(fdt)))
>  			return -FDT_ERR_TRUNCATED;
>  	}
>  
> -	/* Bounds check strings block */
> -	if (!check_block_(hdrsize, fdt_totalsize(fdt),
> -			  fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> -		return -FDT_ERR_TRUNCATED;
> -
>  	return 0;
>  }
>  
> @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  
>  	*nextoffset = -FDT_ERR_TRUNCATED;
>  	tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> -	if (!tagp)
> +	if (!can_assume(VALID_DTB) && !tagp)
>  		return FDT_END; /* premature end */
>  	tag = fdt32_to_cpu(*tagp);
>  	offset += FDT_TAGSIZE;
> @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		do {
>  			p = fdt_offset_ptr(fdt, offset++, 1);
>  		} while (p && (*p != '\0'));
> -		if (!p)
> +		if (!can_assume(VALID_DTB) && !p)
>  			return FDT_END; /* premature end */
>  		break;
>  
>  	case FDT_PROP:
>  		lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> -		if (!lenp)
> +		if (!can_assume(VALID_DTB) && !lenp)
>  			return FDT_END; /* premature end */
>  		/* skip-name offset, length and value */
>  		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index a5c2797..b41083f 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  	const char *nameptr;
>  	int err;
>  
> -	if (((err = fdt_ro_probe_(fdt)) < 0)
> -	    || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> +	if (!can_assume(VALID_DTB)) {
> +		if ((err = fdt_ro_probe_(fdt)) < 0)

Seems like it might be cleaner to include the can_assume() check into
fdt_ro_probe_().

>  			goto fail;
> +		if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> +			goto fail;
> +	}
>  
>  	nameptr = nh->name;
>  
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 8795947..7a41a31 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -13,6 +13,8 @@
>  static int fdt_blocks_misordered_(const void *fdt,
>  				  int mem_rsv_size, int struct_size)
>  {
> +	if (can_assume(VALID_DTB))
> +		return false;

Urgh... the spec doesn't actually specify a block order, so I'm not
sure we can put this one under VALID_DTB.


>  	return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
>  		|| (fdt_off_dt_struct(fdt) <
>  		    (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
>  
>  static int fdt_rw_probe_(void *fdt)
>  {
> +	if (can_assume(VALID_DTB))
> +		return 0;
>  	FDT_RO_PROBE(fdt);
>  
>  	if (fdt_version(fdt) < 17)
> @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
>  #define FDT_RW_PROBE(fdt) \
>  	{ \
>  		int err_; \
> -		if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> +		if (!can_assume(VALID_DTB) && \
> +		    (err_ = fdt_rw_probe_(fdt)) != 0) \


You shouldn't need the test both inside fdt_rw_probe_() and in the
FDT_RW_PROBE() macro, no?

>  			return err_; \
>  	}
>  
> diff --git a/libfdt/fdt_sw.c b/libfdt/fdt_sw.c
> index 76bea22..8fca816 100644
> --- a/libfdt/fdt_sw.c
> +++ b/libfdt/fdt_sw.c
> @@ -12,17 +12,20 @@
>  
>  static int fdt_sw_probe_(void *fdt)
>  {
> -	if (fdt_magic(fdt) == FDT_MAGIC)
> -		return -FDT_ERR_BADSTATE;
> -	else if (fdt_magic(fdt) != FDT_SW_MAGIC)
> -		return -FDT_ERR_BADMAGIC;
> +	if (!can_assume(VALID_DTB)) {
> +		if (fdt_magic(fdt) == FDT_MAGIC)
> +			return -FDT_ERR_BADSTATE;
> +		else if (fdt_magic(fdt) != FDT_SW_MAGIC)
> +			return -FDT_ERR_BADMAGIC;
> +	}
> +
>  	return 0;
>  }
>  
>  #define FDT_SW_PROBE(fdt) \
>  	{ \
>  		int err; \
> -		if ((err = fdt_sw_probe_(fdt)) != 0) \
> +		if (!can_assume(VALID_DTB) && (err = fdt_sw_probe_(fdt)) != 0) \

Same question here.

>  			return err; \
>  	}
>  
> @@ -38,7 +41,7 @@ static int fdt_sw_probe_memrsv_(void *fdt)
>  	if (err)
>  		return err;
>  
> -	if (fdt_off_dt_strings(fdt) != 0)
> +	if (!can_assume(VALID_DTB) && fdt_off_dt_strings(fdt) != 0)
>  		return -FDT_ERR_BADSTATE;
>  	return 0;
>  }
> @@ -46,7 +49,8 @@ static int fdt_sw_probe_memrsv_(void *fdt)
>  #define FDT_SW_PROBE_MEMRSV(fdt) \
>  	{ \
>  		int err; \
> -		if ((err = fdt_sw_probe_memrsv_(fdt)) != 0) \
> +		if (!can_assume(VALID_DTB) && \

And here.

> +		    (err = fdt_sw_probe_memrsv_(fdt)) != 0) \
>  			return err; \
>  	}
>  
> @@ -60,7 +64,11 @@ static int fdt_sw_probe_memrsv_(void *fdt)
>   */
>  static int fdt_sw_probe_struct_(void *fdt)
>  {
> -	int err = fdt_sw_probe_(fdt);
> +	int err;
> +
> +	if (can_assume(VALID_DTB))
> +		return 0;
> +	err = fdt_sw_probe_(fdt);

And here.

>  	if (err)
>  		return err;
>  
> @@ -72,7 +80,8 @@ static int fdt_sw_probe_struct_(void *fdt)
>  #define FDT_SW_PROBE_STRUCT(fdt) \
>  	{ \
>  		int err; \
> -		if ((err = fdt_sw_probe_struct_(fdt)) != 0) \
> +		if (!can_assume(VALID_DTB) && \
> +		    (err = fdt_sw_probe_struct_(fdt)) != 0) \

And here.

>  			return err; \
>  	}
>  
> @@ -151,7 +160,8 @@ int fdt_resize(void *fdt, void *buf, int bufsize)
>  	headsize = fdt_off_dt_struct(fdt) + fdt_size_dt_struct(fdt);
>  	tailsize = fdt_size_dt_strings(fdt);
>  
> -	if ((headsize + tailsize) > fdt_totalsize(fdt))
> +	if (!can_assume(VALID_DTB) &&
> +	    headsize + tailsize > fdt_totalsize(fdt))
>  		return -FDT_ERR_INTERNAL;
>  
>  	if ((headsize + tailsize) > bufsize)
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index 1e31d10..1aa732b 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -14,8 +14,11 @@ int32_t fdt_ro_probe_(const void *fdt);
>  #define FDT_RO_PROBE(fdt)					\
>  	{							\
>  		int32_t totalsize_;				\
> -		if ((totalsize_ = fdt_ro_probe_(fdt)) < 0)	\
> -			return totalsize_;			\
> +		if (!can_assume(VALID_DTB)) {			\
> +			totalsize_ = fdt_ro_probe_(fdt);	\
> +			if (totalsize_ < 0)			\
> +				return totalsize_;		\
> +		}						\

And here.

>  	}
>  
>  int fdt_check_node_offset_(const void *fdt, int offset);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[Simon Glass]

Hi David,

On Sun, 9 Feb 2020 at 23:31, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> > Support ASSUME_VALID_DTB to disable some sanity checks
> >
> > If we assume that the DTB itself is valid then we can skip some checks and
> > save code space. Add various conditions to handle this.
> >
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> >
> > Changes in v5:
> > - Split out VALID_DTB checks into a separate patch
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> >  libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
> >  libfdt/fdt_ro.c          |  7 ++++--
> >  libfdt/fdt_rw.c          |  7 +++++-
> >  libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
> >  libfdt/libfdt_internal.h |  7 ++++--
> >  5 files changed, 64 insertions(+), 37 deletions(-)
> >
> > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > index 3e37a4b..03f2b7d 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
> >
> >       if (fdt_magic(fdt) != FDT_MAGIC)
> >               return -FDT_ERR_BADMAGIC;
> > -     hdrsize = fdt_header_size(fdt);
> >       if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> >           || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> >               return -FDT_ERR_BADVERSION;
> >       if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> >               return -FDT_ERR_BADVERSION;
> > +     hdrsize = fdt_header_size(fdt);
> > +     if (!can_assume(VALID_DTB)) {
> >
> > -     if ((fdt_totalsize(fdt) < hdrsize)
> > -         || (fdt_totalsize(fdt) > INT_MAX))
> > -             return -FDT_ERR_TRUNCATED;
> > -
> > -     /* Bounds check memrsv block */
> > -     if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> > -             return -FDT_ERR_TRUNCATED;
> > +             if ((fdt_totalsize(fdt) < hdrsize)
> > +                 || (fdt_totalsize(fdt) > INT_MAX))
> > +                     return -FDT_ERR_TRUNCATED;
> >
> > -     /* Bounds check structure block */
> > -     if (fdt_version(fdt) < 17) {
> > +             /* Bounds check memrsv block */
> >               if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > -                             fdt_off_dt_struct(fdt)))
> > +                             fdt_off_mem_rsvmap(fdt)))
> >                       return -FDT_ERR_TRUNCATED;
> > -     } else {
> > +     }
> > +
> > +     if (!can_assume(VALID_DTB)) {
> > +             /* Bounds check structure block */
> > +             if (fdt_version(fdt) < 17) {
> > +                     if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > +                                     fdt_off_dt_struct(fdt)))
> > +                             return -FDT_ERR_TRUNCATED;
> > +             } else {
> > +                     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > +                                       fdt_off_dt_struct(fdt),
> > +                                       fdt_size_dt_struct(fdt)))
> > +                             return -FDT_ERR_TRUNCATED;
> > +             }
> > +
> > +             /* Bounds check strings block */
> >               if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > -                               fdt_off_dt_struct(fdt),
> > -                               fdt_size_dt_struct(fdt)))
> > +                               fdt_off_dt_strings(fdt),
> > +                               fdt_size_dt_strings(fdt)))
> >                       return -FDT_ERR_TRUNCATED;
> >       }
> >
> > -     /* Bounds check strings block */
> > -     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > -                       fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> > -             return -FDT_ERR_TRUNCATED;
> > -
> >       return 0;
> >  }
> >
> > @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> >
> >       *nextoffset = -FDT_ERR_TRUNCATED;
> >       tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > -     if (!tagp)
> > +     if (!can_assume(VALID_DTB) && !tagp)
> >               return FDT_END; /* premature end */
> >       tag = fdt32_to_cpu(*tagp);
> >       offset += FDT_TAGSIZE;
> > @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> >               do {
> >                       p = fdt_offset_ptr(fdt, offset++, 1);
> >               } while (p && (*p != '\0'));
> > -             if (!p)
> > +             if (!can_assume(VALID_DTB) && !p)
> >                       return FDT_END; /* premature end */
> >               break;
> >
> >       case FDT_PROP:
> >               lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > -             if (!lenp)
> > +             if (!can_assume(VALID_DTB) && !lenp)
> >                       return FDT_END; /* premature end */
> >               /* skip-name offset, length and value */
> >               offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index a5c2797..b41083f 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> >       const char *nameptr;
> >       int err;
> >
> > -     if (((err = fdt_ro_probe_(fdt)) < 0)
> > -         || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> > +     if (!can_assume(VALID_DTB)) {
> > +             if ((err = fdt_ro_probe_(fdt)) < 0)
>
> Seems like it might be cleaner to include the can_assume() check into
> fdt_ro_probe_().

OK.

>
> >                       goto fail;
> > +             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > +                     goto fail;
> > +     }
> >
> >       nameptr = nh->name;
> >
> > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > index 8795947..7a41a31 100644
> > --- a/libfdt/fdt_rw.c
> > +++ b/libfdt/fdt_rw.c
> > @@ -13,6 +13,8 @@
> >  static int fdt_blocks_misordered_(const void *fdt,
> >                                 int mem_rsv_size, int struct_size)
> >  {
> > +     if (can_assume(VALID_DTB))
> > +             return false;
>
> Urgh... the spec doesn't actually specify a block order, so I'm not
> sure we can put this one under VALID_DTB.

But if this returns true then we return -FDT_ERR_BADLAYOUT in
fdt_rw_probe_() below. So it does seem to be wrong.

>
>
> >       return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> >               || (fdt_off_dt_struct(fdt) <
> >                   (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> >
> >  static int fdt_rw_probe_(void *fdt)
> >  {
> > +     if (can_assume(VALID_DTB))
> > +             return 0;
> >       FDT_RO_PROBE(fdt);
> >
> >       if (fdt_version(fdt) < 17)
> > @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
> >  #define FDT_RW_PROBE(fdt) \
> >       { \
> >               int err_; \
> > -             if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > +             if (!can_assume(VALID_DTB) && \
> > +                 (err_ = fdt_rw_probe_(fdt)) != 0) \
>
>
> You shouldn't need the test both inside fdt_rw_probe_() and in the
> FDT_RW_PROBE() macro, no?

OK. It saves a small amount of code space but we can look at it once
we get something applied...

>
> >                       return err_; \
> >       }
> >

I'll hold off sending a new version until I hear back.

Regards,
Simon

Re: [PATCH v5 4/7] libfdt: Add support for disabling sanity checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8098 bytes --]

On Wed, Feb 05, 2020 at 10:40:31PM -0700, Simon Glass wrote:
> Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
> tree and the parameters to libfdt. This assumption covers that cases where
> the problem could be with either.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
> 
> Changes in v5:
> - Include just VALID_INPUT checks in this patch
> 
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/fdt.c    | 12 +++++----
>  libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
>  2 files changed, 54 insertions(+), 29 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index 03f2b7d..e2c1da0 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  {
>  	unsigned absoffset = offset + fdt_off_dt_struct(fdt);
>  
> -	if ((absoffset < offset)
> -	    || ((absoffset + len) < absoffset)
> -	    || (absoffset + len) > fdt_totalsize(fdt))
> -		return NULL;
> +	if (!can_assume(VALID_INPUT))
> +		if ((absoffset < offset)
> +		    || ((absoffset + len) < absoffset)
> +		    || (absoffset + len) > fdt_totalsize(fdt))
> +			return NULL;
>  
>  	if (fdt_version(fdt) >= 0x11)
>  		if (((offset + len) < offset)
> @@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		return FDT_END;
>  	}
>  
> -	if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> +	if (!can_assume(VALID_INPUT) &&
> +	    !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
>  		return FDT_END; /* premature end */
>  
>  	*nextoffset = FDT_TAGALIGN(offset);
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index b41083f..07c13c9 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
>  	int olen;
>  	const char *p = fdt_get_name(fdt, offset, &olen);
>  
> -	if (!p || olen < len)
> +	if (!p || (!can_assume(VALID_INPUT) && olen < len))

Oof, this one's subtle.  We certainly *can* have olen < len even in
perfectly valid cases.  However, if we're assuming validly \0
terminated strings in the strings block *and* no \0s in s (which you
can with assume(VALID_INPUT)), then the memcmp() will necessarily pick
up that case.

If we also assume memcmp() is the obvious byte-by-byte implementation
then it will stop before accessing beyond the end of the strings block
string.  But... I don't think that's necessarily the case for all C
libraries / runtimes.  So, if this is close to the end of the strings
block, the memcmp() could access beyond the dtb buffer, which is a no
no.

Now, I guess we could have an assume(DUMB_MEMCMP) and/or
assume(READ_ACCESS_SLIGHTLY_BEYOND_THE_DTB_WILL_WORK), but we're
getting really esoteric now.

Is avoiding this one comparison worth it?

>  		/* short match */
>  		return 0;
>  
> @@ -33,17 +33,26 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
>  
>  const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  {
> -	int32_t totalsize = fdt_ro_probe_(fdt);
> -	uint32_t absoffset = stroffset + fdt_off_dt_strings(fdt);
> +	int32_t totalsize;
> +	uint32_t absoffset;
>  	size_t len;
>  	int err;
>  	const char *s, *n;
>  
> +	if (can_assume(VALID_INPUT)) {
> +		s = (const char *)fdt + fdt_off_dt_strings(fdt) + stroffset;
> +
> +		if (lenp)
> +			*lenp = strlen(s);
> +		return s;
> +	}
> +	totalsize = fdt_ro_probe_(fdt);
>  	err = totalsize;
>  	if (totalsize < 0)
>  		goto fail;
>  
>  	err = -FDT_ERR_BADOFFSET;
> +	absoffset = stroffset + fdt_off_dt_strings(fdt);
>  	if (absoffset >= totalsize)
>  		goto fail;
>  	len = totalsize - absoffset;
> @@ -151,10 +160,13 @@ static const struct fdt_reserve_entry *fdt_mem_rsv(const void *fdt, int n)
>  	int offset = n * sizeof(struct fdt_reserve_entry);
>  	int absoffset = fdt_off_mem_rsvmap(fdt) + offset;
>  
> -	if (absoffset < fdt_off_mem_rsvmap(fdt))
> -		return NULL;
> -	if (absoffset > fdt_totalsize(fdt) - sizeof(struct fdt_reserve_entry))
> -		return NULL;
> +	if (!can_assume(VALID_INPUT)) {
> +		if (absoffset < fdt_off_mem_rsvmap(fdt))
> +			return NULL;
> +		if (absoffset > fdt_totalsize(fdt) -
> +		    sizeof(struct fdt_reserve_entry))
> +			return NULL;
> +	}
>  	return fdt_mem_rsv_(fdt, n);
>  }
>  
> @@ -164,7 +176,7 @@ int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
>  
>  	FDT_RO_PROBE(fdt);
>  	re = fdt_mem_rsv(fdt, n);
> -	if (!re)
> +	if (!can_assume(VALID_INPUT) && !re)
>  		return -FDT_ERR_BADOFFSET;
>  
>  	*address = fdt64_ld(&re->address);
> @@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  	if (!can_assume(VALID_DTB)) {
>  		if ((err = fdt_ro_probe_(fdt)) < 0)
>  			goto fail;
> -		if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> -			goto fail;
> +		if (can_assume(VALID_INPUT) &&

That should be !can_assume, no?

> +		    (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> +		goto fail;
>  	}
>  
>  	nameptr = nh->name;
> @@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
>  	int err;
>  	const struct fdt_property *prop;
>  
> -	if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> +	if (!can_assume(VALID_INPUT) &&
> +	    (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
>  		if (lenp)
>  			*lenp = err;
>  		return NULL;
> @@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
>  	     (offset = fdt_next_property_offset(fdt, offset))) {
>  		const struct fdt_property *prop;
>  
> -		if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> +		prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> +		if (!can_assume(VALID_INPUT) && !prop) {
>  			offset = -FDT_ERR_INTERNAL;

So, arguably you could put this one under a weaker assumption flag.
Basicaly FDT_ERR_INTERNAL errors should *never* happen, even with bad
input - they're basically assert()s, except I didn't want to rely on
the runtime things that assert() needs.

>  			break;
>  		}
> @@ -464,14 +479,19 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  	if (namep) {
>  		const char *name;
>  		int namelen;
> -		name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
> -				      &namelen);
> -		if (!name) {
> -			if (lenp)
> -				*lenp = namelen;
> -			return NULL;
> +
> +		if (!can_assume(VALID_INPUT)) {
> +			name = fdt_get_string(fdt, fdt32_ld(&prop->nameoff),
> +					      &namelen);
> +			if (!name) {
> +				if (lenp)
> +					*lenp = namelen;
> +				return NULL;
> +			}
> +			*namep = name;
> +		} else {
> +			*namep = fdt_string(fdt, fdt32_ld(&prop->nameoff));
>  		}
> -		*namep = name;
>  	}
>  
>  	/* Handle realignment */
> @@ -601,10 +621,12 @@ int fdt_supernode_atdepth_offset(const void *fdt, int nodeoffset,
>  		}
>  	}
>  
> -	if ((offset == -FDT_ERR_NOTFOUND) || (offset >= 0))
> -		return -FDT_ERR_BADOFFSET;
> -	else if (offset == -FDT_ERR_BADOFFSET)
> -		return -FDT_ERR_BADSTRUCTURE;
> +	if (!can_assume(VALID_INPUT)) {
> +		if ((offset == -FDT_ERR_NOTFOUND) || (offset >= 0))
> +			return -FDT_ERR_BADOFFSET;
> +		else if (offset == -FDT_ERR_BADOFFSET)
> +			return -FDT_ERR_BADSTRUCTURE;
> +	}
>  
>  	return offset; /* error from fdt_next_node() */
>  }
> @@ -616,7 +638,8 @@ int fdt_node_depth(const void *fdt, int nodeoffset)
>  
>  	err = fdt_supernode_atdepth_offset(fdt, nodeoffset, 0, &nodedepth);
>  	if (err)
> -		return (err < 0) ? err : -FDT_ERR_INTERNAL;
> +		return (can_assume(VALID_INPUT) || err < 0) ? err :
> +			-FDT_ERR_INTERNAL;
>  	return nodedepth;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9070 bytes --]

On Mon, Feb 10, 2020 at 06:04:47PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Sun, 9 Feb 2020 at 23:31, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> > > Support ASSUME_VALID_DTB to disable some sanity checks
> > >
> > > If we assume that the DTB itself is valid then we can skip some checks and
> > > save code space. Add various conditions to handle this.
> > >
> > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > ---
> > >
> > > Changes in v5:
> > > - Split out VALID_DTB checks into a separate patch
> > >
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > >  libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
> > >  libfdt/fdt_ro.c          |  7 ++++--
> > >  libfdt/fdt_rw.c          |  7 +++++-
> > >  libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
> > >  libfdt/libfdt_internal.h |  7 ++++--
> > >  5 files changed, 64 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > index 3e37a4b..03f2b7d 100644
> > > --- a/libfdt/fdt.c
> > > +++ b/libfdt/fdt.c
> > > @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
> > >
> > >       if (fdt_magic(fdt) != FDT_MAGIC)
> > >               return -FDT_ERR_BADMAGIC;
> > > -     hdrsize = fdt_header_size(fdt);
> > >       if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > >           || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > >               return -FDT_ERR_BADVERSION;
> > >       if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > >               return -FDT_ERR_BADVERSION;
> > > +     hdrsize = fdt_header_size(fdt);
> > > +     if (!can_assume(VALID_DTB)) {
> > >
> > > -     if ((fdt_totalsize(fdt) < hdrsize)
> > > -         || (fdt_totalsize(fdt) > INT_MAX))
> > > -             return -FDT_ERR_TRUNCATED;
> > > -
> > > -     /* Bounds check memrsv block */
> > > -     if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> > > -             return -FDT_ERR_TRUNCATED;
> > > +             if ((fdt_totalsize(fdt) < hdrsize)
> > > +                 || (fdt_totalsize(fdt) > INT_MAX))
> > > +                     return -FDT_ERR_TRUNCATED;
> > >
> > > -     /* Bounds check structure block */
> > > -     if (fdt_version(fdt) < 17) {
> > > +             /* Bounds check memrsv block */
> > >               if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > -                             fdt_off_dt_struct(fdt)))
> > > +                             fdt_off_mem_rsvmap(fdt)))
> > >                       return -FDT_ERR_TRUNCATED;
> > > -     } else {
> > > +     }
> > > +
> > > +     if (!can_assume(VALID_DTB)) {
> > > +             /* Bounds check structure block */
> > > +             if (fdt_version(fdt) < 17) {
> > > +                     if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > +                                     fdt_off_dt_struct(fdt)))
> > > +                             return -FDT_ERR_TRUNCATED;
> > > +             } else {
> > > +                     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > +                                       fdt_off_dt_struct(fdt),
> > > +                                       fdt_size_dt_struct(fdt)))
> > > +                             return -FDT_ERR_TRUNCATED;
> > > +             }
> > > +
> > > +             /* Bounds check strings block */
> > >               if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > -                               fdt_off_dt_struct(fdt),
> > > -                               fdt_size_dt_struct(fdt)))
> > > +                               fdt_off_dt_strings(fdt),
> > > +                               fdt_size_dt_strings(fdt)))
> > >                       return -FDT_ERR_TRUNCATED;
> > >       }
> > >
> > > -     /* Bounds check strings block */
> > > -     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > -                       fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> > > -             return -FDT_ERR_TRUNCATED;
> > > -
> > >       return 0;
> > >  }
> > >
> > > @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > >
> > >       *nextoffset = -FDT_ERR_TRUNCATED;
> > >       tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > -     if (!tagp)
> > > +     if (!can_assume(VALID_DTB) && !tagp)
> > >               return FDT_END; /* premature end */
> > >       tag = fdt32_to_cpu(*tagp);
> > >       offset += FDT_TAGSIZE;
> > > @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > >               do {
> > >                       p = fdt_offset_ptr(fdt, offset++, 1);
> > >               } while (p && (*p != '\0'));
> > > -             if (!p)
> > > +             if (!can_assume(VALID_DTB) && !p)
> > >                       return FDT_END; /* premature end */
> > >               break;
> > >
> > >       case FDT_PROP:
> > >               lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > > -             if (!lenp)
> > > +             if (!can_assume(VALID_DTB) && !lenp)
> > >                       return FDT_END; /* premature end */
> > >               /* skip-name offset, length and value */
> > >               offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index a5c2797..b41083f 100644
> > > --- a/libfdt/fdt_ro.c
> > > +++ b/libfdt/fdt_ro.c
> > > @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > >       const char *nameptr;
> > >       int err;
> > >
> > > -     if (((err = fdt_ro_probe_(fdt)) < 0)
> > > -         || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> > > +     if (!can_assume(VALID_DTB)) {
> > > +             if ((err = fdt_ro_probe_(fdt)) < 0)
> >
> > Seems like it might be cleaner to include the can_assume() check into
> > fdt_ro_probe_().
> 
> OK.
> 
> >
> > >                       goto fail;
> > > +             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > +                     goto fail;
> > > +     }
> > >
> > >       nameptr = nh->name;
> > >
> > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > index 8795947..7a41a31 100644
> > > --- a/libfdt/fdt_rw.c
> > > +++ b/libfdt/fdt_rw.c
> > > @@ -13,6 +13,8 @@
> > >  static int fdt_blocks_misordered_(const void *fdt,
> > >                                 int mem_rsv_size, int struct_size)
> > >  {
> > > +     if (can_assume(VALID_DTB))
> > > +             return false;
> >
> > Urgh... the spec doesn't actually specify a block order, so I'm not
> > sure we can put this one under VALID_DTB.
> 
> But if this returns true then we return -FDT_ERR_BADLAYOUT in
> fdt_rw_probe_() below. So it does seem to be wrong.

Ah, right.  So, that's true for all cases, except the call in
fdt_open_into().  The idea is that you have to call fdt_open_into()
before any of the other rw functions.  fdt_open_into() will re-order
the blocks, if necessary, subsequent functions then require them in
the "standard" order.

So for the cases other than open_into() you can probably put this
under VALID_INPUT, I think it's reasonable to include "you've called
prerequisite functions correctly" under requiring suitable input.

You almost certainly also want to elide the block reordering code in
fdt_open_into(), though.  That one probably wants another flag

> > >       return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> > >               || (fdt_off_dt_struct(fdt) <
> > >                   (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > >
> > >  static int fdt_rw_probe_(void *fdt)
> > >  {
> > > +     if (can_assume(VALID_DTB))
> > > +             return 0;
> > >       FDT_RO_PROBE(fdt);
> > >
> > >       if (fdt_version(fdt) < 17)
> > > @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
> > >  #define FDT_RW_PROBE(fdt) \
> > >       { \
> > >               int err_; \
> > > -             if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > > +             if (!can_assume(VALID_DTB) && \
> > > +                 (err_ = fdt_rw_probe_(fdt)) != 0) \
> >
> >
> > You shouldn't need the test both inside fdt_rw_probe_() and in the
> > FDT_RW_PROBE() macro, no?
> 
> OK. It saves a small amount of code space but we can look at it once
> we get something applied...

Ok, do you know why that is?

> 
> >
> > >                       return err_; \
> > >       }
> > >
> 
> I'll hold off sending a new version until I hear back.
> 
> Regards,
> Simon
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 6/7] libfdt: Add support for disabling version checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7436 bytes --]

On Wed, Feb 05, 2020 at 10:40:33PM -0700, Simon Glass wrote:
> Allow enabling FDT_ASSUME_LATEST to disable version checks.
> 
> Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Reviewed-by: David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org>

> ---
> 
> Changes in v5: None
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
> 
>  libfdt/fdt.c    | 34 +++++++++++++++++++++-------------
>  libfdt/fdt_ro.c | 16 ++++++++--------
>  libfdt/fdt_rw.c |  6 +++---
>  3 files changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> index e2c1da0..0ac8736 100644
> --- a/libfdt/fdt.c
> +++ b/libfdt/fdt.c
> @@ -21,10 +21,13 @@ int32_t fdt_ro_probe_(const void *fdt)
>  
>  	if (fdt_magic(fdt) == FDT_MAGIC) {
>  		/* Complete tree */
> -		if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> -			return -FDT_ERR_BADVERSION;
> -		if (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION)
> -			return -FDT_ERR_BADVERSION;
> +		if (!can_assume(LATEST)) {
> +			if (fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> +				return -FDT_ERR_BADVERSION;
> +			if (fdt_last_comp_version(fdt) >
> +					FDT_LAST_SUPPORTED_VERSION)
> +				return -FDT_ERR_BADVERSION;
> +		}
>  	} else if (fdt_magic(fdt) == FDT_SW_MAGIC) {
>  		/* Unfinished sequential-write blob */
>  		if (fdt_size_dt_struct(fdt) == 0)
> @@ -72,7 +75,8 @@ size_t fdt_header_size_(uint32_t version)
>  
>  size_t fdt_header_size(const void *fdt)
>  {
> -	return fdt_header_size_(fdt_version(fdt));
> +	return can_assume(LATEST) ? FDT_V17_SIZE :
> +		fdt_header_size_(fdt_version(fdt));
>  }
>  
>  int fdt_check_header(const void *fdt)
> @@ -81,11 +85,14 @@ int fdt_check_header(const void *fdt)
>  
>  	if (fdt_magic(fdt) != FDT_MAGIC)
>  		return -FDT_ERR_BADMAGIC;
> -	if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> -	    || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> -		return -FDT_ERR_BADVERSION;
> -	if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> -		return -FDT_ERR_BADVERSION;
> +	if (!can_assume(LATEST)) {
> +		if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> +		    || (fdt_last_comp_version(fdt) >
> +			FDT_LAST_SUPPORTED_VERSION))
> +			return -FDT_ERR_BADVERSION;
> +		if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> +			return -FDT_ERR_BADVERSION;
> +	}
>  	hdrsize = fdt_header_size(fdt);
>  	if (!can_assume(VALID_DTB)) {
>  
> @@ -101,7 +108,7 @@ int fdt_check_header(const void *fdt)
>  
>  	if (!can_assume(VALID_DTB)) {
>  		/* Bounds check structure block */
> -		if (fdt_version(fdt) < 17) {
> +		if (!can_assume(LATEST) && fdt_version(fdt) < 17) {
>  			if (!check_off_(hdrsize, fdt_totalsize(fdt),
>  					fdt_off_dt_struct(fdt)))
>  				return -FDT_ERR_TRUNCATED;
> @@ -132,7 +139,7 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
>  		    || (absoffset + len) > fdt_totalsize(fdt))
>  			return NULL;
>  
> -	if (fdt_version(fdt) >= 0x11)
> +	if (can_assume(LATEST) || fdt_version(fdt) >= 0x11)
>  		if (((offset + len) < offset)
>  		    || ((offset + len) > fdt_size_dt_struct(fdt)))
>  			return NULL;
> @@ -172,7 +179,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
>  		/* skip-name offset, length and value */
>  		offset += sizeof(struct fdt_property) - FDT_TAGSIZE
>  			+ fdt32_to_cpu(*lenp);
> -		if (fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
> +		if (!can_assume(LATEST) &&
> +		    fdt_version(fdt) < 0x10 && fdt32_to_cpu(*lenp) >= 8 &&
>  		    ((offset - fdt32_to_cpu(*lenp)) % 8) != 0)
>  			offset += 4;
>  		break;
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 07c13c9..63cb60e 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -60,7 +60,7 @@ const char *fdt_get_string(const void *fdt, int stroffset, int *lenp)
>  	if (fdt_magic(fdt) == FDT_MAGIC) {
>  		if (stroffset < 0)
>  			goto fail;
> -		if (fdt_version(fdt) >= 17) {
> +		if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
>  			if (stroffset >= fdt_size_dt_strings(fdt))
>  				goto fail;
>  			if ((fdt_size_dt_strings(fdt) - stroffset) < len)
> @@ -311,7 +311,7 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
>  
>  	nameptr = nh->name;
>  
> -	if (fdt_version(fdt) < 0x10) {
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
>  		/*
>  		 * For old FDT versions, match the naming conventions of V16:
>  		 * give only the leaf name (after all /). The actual tree
> @@ -384,7 +384,7 @@ const struct fdt_property *fdt_get_property_by_offset(const void *fdt,
>  	/* Prior to version 16, properties may need realignment
>  	 * and this API does not work. fdt_getprop_*() will, however. */
>  
> -	if (fdt_version(fdt) < 0x10) {
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
>  		if (lenp)
>  			*lenp = -FDT_ERR_BADVERSION;
>  		return NULL;
> @@ -431,7 +431,7 @@ const struct fdt_property *fdt_get_property_namelen(const void *fdt,
>  {
>  	/* Prior to version 16, properties may need realignment
>  	 * and this API does not work. fdt_getprop_*() will, however. */
> -	if (fdt_version(fdt) < 0x10) {
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10) {
>  		if (lenp)
>  			*lenp = -FDT_ERR_BADVERSION;
>  		return NULL;
> @@ -462,8 +462,8 @@ const void *fdt_getprop_namelen(const void *fdt, int nodeoffset,
>  		return NULL;
>  
>  	/* Handle realignment */
> -	if (fdt_version(fdt) < 0x10 && (poffset + sizeof(*prop)) % 8 &&
> -	    fdt32_ld(&prop->len) >= 8)
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
> +	    (poffset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
>  		return prop->data + 4;
>  	return prop->data;
>  }
> @@ -495,8 +495,8 @@ const void *fdt_getprop_by_offset(const void *fdt, int offset,
>  	}
>  
>  	/* Handle realignment */
> -	if (fdt_version(fdt) < 0x10 && (offset + sizeof(*prop)) % 8 &&
> -	    fdt32_ld(&prop->len) >= 8)
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 0x10 &&
> +	    (offset + sizeof(*prop)) % 8 && fdt32_ld(&prop->len) >= 8)
>  		return prop->data + 4;
>  	return prop->data;
>  }
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 75f70d9..a18e123 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -30,12 +30,12 @@ static int fdt_rw_probe_(void *fdt)
>  		return 0;
>  	FDT_RO_PROBE(fdt);
>  
> -	if (fdt_version(fdt) < 17)
> +	if (!can_assume(LATEST) && fdt_version(fdt) < 17)
>  		return -FDT_ERR_BADVERSION;
>  	if (fdt_blocks_misordered_(fdt, sizeof(struct fdt_reserve_entry),
>  				   fdt_size_dt_struct(fdt)))
>  		return -FDT_ERR_BADLAYOUT;
> -	if (fdt_version(fdt) > 17)
> +	if (!can_assume(LATEST) && fdt_version(fdt) > 17)
>  		fdt_set_version(fdt, 17);
>  
>  	return 0;
> @@ -428,7 +428,7 @@ int fdt_open_into(const void *fdt, void *buf, int bufsize)
>  	mem_rsv_size = (fdt_num_mem_rsv(fdt)+1)
>  		* sizeof(struct fdt_reserve_entry);
>  
> -	if (fdt_version(fdt) >= 17) {
> +	if (can_assume(LATEST) || fdt_version(fdt) >= 17) {
>  		struct_size = fdt_size_dt_struct(fdt);
>  	} else {
>  		struct_size = 0;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[Simon Glass]

Hi David,

On Mon, 10 Feb 2020 at 18:43, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Mon, Feb 10, 2020 at 06:04:47PM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Sun, 9 Feb 2020 at 23:31, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> > > > Support ASSUME_VALID_DTB to disable some sanity checks
> > > >
> > > > If we assume that the DTB itself is valid then we can skip some checks and
> > > > save code space. Add various conditions to handle this.
> > > >
> > > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > ---
> > > >
> > > > Changes in v5:
> > > > - Split out VALID_DTB checks into a separate patch
> > > >
> > > > Changes in v4: None
> > > > Changes in v3: None
> > > > Changes in v2: None
> > > >
> > > >  libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
> > > >  libfdt/fdt_ro.c          |  7 ++++--
> > > >  libfdt/fdt_rw.c          |  7 +++++-
> > > >  libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
> > > >  libfdt/libfdt_internal.h |  7 ++++--
> > > >  5 files changed, 64 insertions(+), 37 deletions(-)
> > > >
> > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > > index 3e37a4b..03f2b7d 100644
> > > > --- a/libfdt/fdt.c
> > > > +++ b/libfdt/fdt.c
> > > > @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
> > > >
> > > >       if (fdt_magic(fdt) != FDT_MAGIC)
> > > >               return -FDT_ERR_BADMAGIC;
> > > > -     hdrsize = fdt_header_size(fdt);
> > > >       if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > >           || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > > >               return -FDT_ERR_BADVERSION;
> > > >       if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > >               return -FDT_ERR_BADVERSION;
> > > > +     hdrsize = fdt_header_size(fdt);
> > > > +     if (!can_assume(VALID_DTB)) {
> > > >
> > > > -     if ((fdt_totalsize(fdt) < hdrsize)
> > > > -         || (fdt_totalsize(fdt) > INT_MAX))
> > > > -             return -FDT_ERR_TRUNCATED;
> > > > -
> > > > -     /* Bounds check memrsv block */
> > > > -     if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> > > > -             return -FDT_ERR_TRUNCATED;
> > > > +             if ((fdt_totalsize(fdt) < hdrsize)
> > > > +                 || (fdt_totalsize(fdt) > INT_MAX))
> > > > +                     return -FDT_ERR_TRUNCATED;
> > > >
> > > > -     /* Bounds check structure block */
> > > > -     if (fdt_version(fdt) < 17) {
> > > > +             /* Bounds check memrsv block */
> > > >               if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > > -                             fdt_off_dt_struct(fdt)))
> > > > +                             fdt_off_mem_rsvmap(fdt)))
> > > >                       return -FDT_ERR_TRUNCATED;
> > > > -     } else {
> > > > +     }
> > > > +
> > > > +     if (!can_assume(VALID_DTB)) {
> > > > +             /* Bounds check structure block */
> > > > +             if (fdt_version(fdt) < 17) {
> > > > +                     if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > > +                                     fdt_off_dt_struct(fdt)))
> > > > +                             return -FDT_ERR_TRUNCATED;
> > > > +             } else {
> > > > +                     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > +                                       fdt_off_dt_struct(fdt),
> > > > +                                       fdt_size_dt_struct(fdt)))
> > > > +                             return -FDT_ERR_TRUNCATED;
> > > > +             }
> > > > +
> > > > +             /* Bounds check strings block */
> > > >               if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > -                               fdt_off_dt_struct(fdt),
> > > > -                               fdt_size_dt_struct(fdt)))
> > > > +                               fdt_off_dt_strings(fdt),
> > > > +                               fdt_size_dt_strings(fdt)))
> > > >                       return -FDT_ERR_TRUNCATED;
> > > >       }
> > > >
> > > > -     /* Bounds check strings block */
> > > > -     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > -                       fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> > > > -             return -FDT_ERR_TRUNCATED;
> > > > -
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > >
> > > >       *nextoffset = -FDT_ERR_TRUNCATED;
> > > >       tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > > -     if (!tagp)
> > > > +     if (!can_assume(VALID_DTB) && !tagp)
> > > >               return FDT_END; /* premature end */
> > > >       tag = fdt32_to_cpu(*tagp);
> > > >       offset += FDT_TAGSIZE;
> > > > @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > >               do {
> > > >                       p = fdt_offset_ptr(fdt, offset++, 1);
> > > >               } while (p && (*p != '\0'));
> > > > -             if (!p)
> > > > +             if (!can_assume(VALID_DTB) && !p)
> > > >                       return FDT_END; /* premature end */
> > > >               break;
> > > >
> > > >       case FDT_PROP:
> > > >               lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > > > -             if (!lenp)
> > > > +             if (!can_assume(VALID_DTB) && !lenp)
> > > >                       return FDT_END; /* premature end */
> > > >               /* skip-name offset, length and value */
> > > >               offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > index a5c2797..b41083f 100644
> > > > --- a/libfdt/fdt_ro.c
> > > > +++ b/libfdt/fdt_ro.c
> > > > @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > >       const char *nameptr;
> > > >       int err;
> > > >
> > > > -     if (((err = fdt_ro_probe_(fdt)) < 0)
> > > > -         || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> > > > +     if (!can_assume(VALID_DTB)) {
> > > > +             if ((err = fdt_ro_probe_(fdt)) < 0)
> > >
> > > Seems like it might be cleaner to include the can_assume() check into
> > > fdt_ro_probe_().
> >
> > OK.
> >
> > >
> > > >                       goto fail;
> > > > +             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > +                     goto fail;
> > > > +     }
> > > >
> > > >       nameptr = nh->name;
> > > >
> > > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > > index 8795947..7a41a31 100644
> > > > --- a/libfdt/fdt_rw.c
> > > > +++ b/libfdt/fdt_rw.c
> > > > @@ -13,6 +13,8 @@
> > > >  static int fdt_blocks_misordered_(const void *fdt,
> > > >                                 int mem_rsv_size, int struct_size)
> > > >  {
> > > > +     if (can_assume(VALID_DTB))
> > > > +             return false;
> > >
> > > Urgh... the spec doesn't actually specify a block order, so I'm not
> > > sure we can put this one under VALID_DTB.
> >
> > But if this returns true then we return -FDT_ERR_BADLAYOUT in
> > fdt_rw_probe_() below. So it does seem to be wrong.
>
> Ah, right.  So, that's true for all cases, except the call in
> fdt_open_into().  The idea is that you have to call fdt_open_into()
> before any of the other rw functions.  fdt_open_into() will re-order
> the blocks, if necessary, subsequent functions then require them in
> the "standard" order.
>
> So for the cases other than open_into() you can probably put this
> under VALID_INPUT, I think it's reasonable to include "you've called
> prerequisite functions correctly" under requiring suitable input.
>
> You almost certainly also want to elide the block reordering code in
> fdt_open_into(), though.  That one probably wants another flag

OK, v6.

>
> > > >       return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> > > >               || (fdt_off_dt_struct(fdt) <
> > > >                   (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > > >
> > > >  static int fdt_rw_probe_(void *fdt)
> > > >  {
> > > > +     if (can_assume(VALID_DTB))
> > > > +             return 0;
> > > >       FDT_RO_PROBE(fdt);
> > > >
> > > >       if (fdt_version(fdt) < 17)
> > > > @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
> > > >  #define FDT_RW_PROBE(fdt) \
> > > >       { \
> > > >               int err_; \
> > > > -             if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > > > +             if (!can_assume(VALID_DTB) && \
> > > > +                 (err_ = fdt_rw_probe_(fdt)) != 0) \
> > >
> > >
> > > You shouldn't need the test both inside fdt_rw_probe_() and in the
> > > FDT_RW_PROBE() macro, no?
> >
> > OK. It saves a small amount of code space but we can look at it once
> > we get something applied...
>
> Ok, do you know why that is?

Well just that if fdt_rw_probe_() is called it consumes a few bytes.
If it is never called it won't be included in the SPL image.

Regards,
Simon

Re: [PATCH v5 4/7] libfdt: Add support for disabling sanity checks

[Simon Glass]

Hi David,

On Mon, 10 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Wed, Feb 05, 2020 at 10:40:31PM -0700, Simon Glass wrote:
> > Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
> > tree and the parameters to libfdt. This assumption covers that cases where
> > the problem could be with either.
> >
> > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > ---
> >
> > Changes in v5:
> > - Include just VALID_INPUT checks in this patch
> >
> > Changes in v4: None
> > Changes in v3: None
> > Changes in v2: None
> >
> >  libfdt/fdt.c    | 12 +++++----
> >  libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
> >  2 files changed, 54 insertions(+), 29 deletions(-)
> >
> > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > index 03f2b7d..e2c1da0 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> >  {
> >       unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> >
> > -     if ((absoffset < offset)
> > -         || ((absoffset + len) < absoffset)
> > -         || (absoffset + len) > fdt_totalsize(fdt))
> > -             return NULL;
> > +     if (!can_assume(VALID_INPUT))
> > +             if ((absoffset < offset)
> > +                 || ((absoffset + len) < absoffset)
> > +                 || (absoffset + len) > fdt_totalsize(fdt))
> > +                     return NULL;
> >
> >       if (fdt_version(fdt) >= 0x11)
> >               if (((offset + len) < offset)
> > @@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> >               return FDT_END;
> >       }
> >
> > -     if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > +     if (!can_assume(VALID_INPUT) &&
> > +         !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> >               return FDT_END; /* premature end */
> >
> >       *nextoffset = FDT_TAGALIGN(offset);
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index b41083f..07c13c9 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
> >       int olen;
> >       const char *p = fdt_get_name(fdt, offset, &olen);
> >
> > -     if (!p || olen < len)
> > +     if (!p || (!can_assume(VALID_INPUT) && olen < len))
>
> Oof, this one's subtle.  We certainly *can* have olen < len even in
> perfectly valid cases.  However, if we're assuming validly \0
> terminated strings in the strings block *and* no \0s in s (which you
> can with assume(VALID_INPUT)), then the memcmp() will necessarily pick
> up that case.
>
> If we also assume memcmp() is the obvious byte-by-byte implementation
> then it will stop before accessing beyond the end of the strings block
> string.  But... I don't think that's necessarily the case for all C
> libraries / runtimes.  So, if this is close to the end of the strings
> block, the memcmp() could access beyond the dtb buffer, which is a no
> no.
>
> Now, I guess we could have an assume(DUMB_MEMCMP) and/or
> assume(READ_ACCESS_SLIGHTLY_BEYOND_THE_DTB_WILL_WORK), but we're
> getting really esoteric now.
>
> Is avoiding this one comparison worth it?

For further context see this commit:

f1879e1 Add limited read-only support for older (V2 and V3) device
tree to libfdt.

Before that we assumed that it was safe to do the memcmp(), so I
figured we could revert the behaviour with this assumption.

Another point is that fdt_subnode_offset_namelen() should probably not
allow namelen to be less than strlen(name). Should we add a comment
and check for that?

Also I don't think I fully understand fdt_nodename_eq_(). It doesn't
have a function comment so it's not really clear what it is supposed
to do. But if I call it with:

fdt_nodename_eq_(fdt, offset, s="ernie", len=5)

and say that fdt_get_name() returns "fred" with olen=4.

Now olen < len is true, so this function will return 0, indicating a
match. But there is no match. What am I missing?

Anyway I agree it doesn't seem worth it, but I'd like to get some
comments into these functions.

>
> >               /* short match */
> >               return 0;
> >

[..]

> > @@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> >       if (!can_assume(VALID_DTB)) {
> >               if ((err = fdt_ro_probe_(fdt)) < 0)
> >                       goto fail;
> > -             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > -                     goto fail;
> > +             if (can_assume(VALID_INPUT) &&
>
> That should be !can_assume, no?

Yes, although I've dropped it in v6 since the check is now in
fdt_check_node_offset_().
>
> > +                 (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > +             goto fail;
> >       }
> >
> >       nameptr = nh->name;
> > @@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
> >       int err;
> >       const struct fdt_property *prop;
> >
> > -     if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > +     if (!can_assume(VALID_INPUT) &&
> > +         (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> >               if (lenp)
> >                       *lenp = err;
> >               return NULL;
> > @@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
> >            (offset = fdt_next_property_offset(fdt, offset))) {
> >               const struct fdt_property *prop;
> >
> > -             if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> > +             prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> > +             if (!can_assume(VALID_INPUT) && !prop) {
> >                       offset = -FDT_ERR_INTERNAL;
>
> So, arguably you could put this one under a weaker assumption flag.
> Basicaly FDT_ERR_INTERNAL errors should *never* happen, even with bad
> input - they're basically assert()s, except I didn't want to rely on
> the runtime things that assert() needs.

Yes I see. The fdt_first/next_property_offset() functions should never
return anything invalid.

Regards,
Simon

Re: [PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10248 bytes --]

On Tue, Feb 11, 2020 at 01:08:41PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Mon, 10 Feb 2020 at 18:43, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Mon, Feb 10, 2020 at 06:04:47PM -0700, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Sun, 9 Feb 2020 at 23:31, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6iEOx6SR5NKn@public.gmane.orgu> wrote:
> > > >
> > > > On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> > > > > Support ASSUME_VALID_DTB to disable some sanity checks
> > > > >
> > > > > If we assume that the DTB itself is valid then we can skip some checks and
> > > > > save code space. Add various conditions to handle this.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > > ---
> > > > >
> > > > > Changes in v5:
> > > > > - Split out VALID_DTB checks into a separate patch
> > > > >
> > > > > Changes in v4: None
> > > > > Changes in v3: None
> > > > > Changes in v2: None
> > > > >
> > > > >  libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
> > > > >  libfdt/fdt_ro.c          |  7 ++++--
> > > > >  libfdt/fdt_rw.c          |  7 +++++-
> > > > >  libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
> > > > >  libfdt/libfdt_internal.h |  7 ++++--
> > > > >  5 files changed, 64 insertions(+), 37 deletions(-)
> > > > >
> > > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > > > index 3e37a4b..03f2b7d 100644
> > > > > --- a/libfdt/fdt.c
> > > > > +++ b/libfdt/fdt.c
> > > > > @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
> > > > >
> > > > >       if (fdt_magic(fdt) != FDT_MAGIC)
> > > > >               return -FDT_ERR_BADMAGIC;
> > > > > -     hdrsize = fdt_header_size(fdt);
> > > > >       if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > > >           || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > > > >               return -FDT_ERR_BADVERSION;
> > > > >       if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > > >               return -FDT_ERR_BADVERSION;
> > > > > +     hdrsize = fdt_header_size(fdt);
> > > > > +     if (!can_assume(VALID_DTB)) {
> > > > >
> > > > > -     if ((fdt_totalsize(fdt) < hdrsize)
> > > > > -         || (fdt_totalsize(fdt) > INT_MAX))
> > > > > -             return -FDT_ERR_TRUNCATED;
> > > > > -
> > > > > -     /* Bounds check memrsv block */
> > > > > -     if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> > > > > -             return -FDT_ERR_TRUNCATED;
> > > > > +             if ((fdt_totalsize(fdt) < hdrsize)
> > > > > +                 || (fdt_totalsize(fdt) > INT_MAX))
> > > > > +                     return -FDT_ERR_TRUNCATED;
> > > > >
> > > > > -     /* Bounds check structure block */
> > > > > -     if (fdt_version(fdt) < 17) {
> > > > > +             /* Bounds check memrsv block */
> > > > >               if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > > > -                             fdt_off_dt_struct(fdt)))
> > > > > +                             fdt_off_mem_rsvmap(fdt)))
> > > > >                       return -FDT_ERR_TRUNCATED;
> > > > > -     } else {
> > > > > +     }
> > > > > +
> > > > > +     if (!can_assume(VALID_DTB)) {
> > > > > +             /* Bounds check structure block */
> > > > > +             if (fdt_version(fdt) < 17) {
> > > > > +                     if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > > > +                                     fdt_off_dt_struct(fdt)))
> > > > > +                             return -FDT_ERR_TRUNCATED;
> > > > > +             } else {
> > > > > +                     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > > +                                       fdt_off_dt_struct(fdt),
> > > > > +                                       fdt_size_dt_struct(fdt)))
> > > > > +                             return -FDT_ERR_TRUNCATED;
> > > > > +             }
> > > > > +
> > > > > +             /* Bounds check strings block */
> > > > >               if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > > -                               fdt_off_dt_struct(fdt),
> > > > > -                               fdt_size_dt_struct(fdt)))
> > > > > +                               fdt_off_dt_strings(fdt),
> > > > > +                               fdt_size_dt_strings(fdt)))
> > > > >                       return -FDT_ERR_TRUNCATED;
> > > > >       }
> > > > >
> > > > > -     /* Bounds check strings block */
> > > > > -     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > > -                       fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> > > > > -             return -FDT_ERR_TRUNCATED;
> > > > > -
> > > > >       return 0;
> > > > >  }
> > > > >
> > > > > @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > > >
> > > > >       *nextoffset = -FDT_ERR_TRUNCATED;
> > > > >       tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > > > -     if (!tagp)
> > > > > +     if (!can_assume(VALID_DTB) && !tagp)
> > > > >               return FDT_END; /* premature end */
> > > > >       tag = fdt32_to_cpu(*tagp);
> > > > >       offset += FDT_TAGSIZE;
> > > > > @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > > >               do {
> > > > >                       p = fdt_offset_ptr(fdt, offset++, 1);
> > > > >               } while (p && (*p != '\0'));
> > > > > -             if (!p)
> > > > > +             if (!can_assume(VALID_DTB) && !p)
> > > > >                       return FDT_END; /* premature end */
> > > > >               break;
> > > > >
> > > > >       case FDT_PROP:
> > > > >               lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > > > > -             if (!lenp)
> > > > > +             if (!can_assume(VALID_DTB) && !lenp)
> > > > >                       return FDT_END; /* premature end */
> > > > >               /* skip-name offset, length and value */
> > > > >               offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> > > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > > index a5c2797..b41083f 100644
> > > > > --- a/libfdt/fdt_ro.c
> > > > > +++ b/libfdt/fdt_ro.c
> > > > > @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > > >       const char *nameptr;
> > > > >       int err;
> > > > >
> > > > > -     if (((err = fdt_ro_probe_(fdt)) < 0)
> > > > > -         || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> > > > > +     if (!can_assume(VALID_DTB)) {
> > > > > +             if ((err = fdt_ro_probe_(fdt)) < 0)
> > > >
> > > > Seems like it might be cleaner to include the can_assume() check into
> > > > fdt_ro_probe_().
> > >
> > > OK.
> > >
> > > >
> > > > >                       goto fail;
> > > > > +             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > > +                     goto fail;
> > > > > +     }
> > > > >
> > > > >       nameptr = nh->name;
> > > > >
> > > > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > > > index 8795947..7a41a31 100644
> > > > > --- a/libfdt/fdt_rw.c
> > > > > +++ b/libfdt/fdt_rw.c
> > > > > @@ -13,6 +13,8 @@
> > > > >  static int fdt_blocks_misordered_(const void *fdt,
> > > > >                                 int mem_rsv_size, int struct_size)
> > > > >  {
> > > > > +     if (can_assume(VALID_DTB))
> > > > > +             return false;
> > > >
> > > > Urgh... the spec doesn't actually specify a block order, so I'm not
> > > > sure we can put this one under VALID_DTB.
> > >
> > > But if this returns true then we return -FDT_ERR_BADLAYOUT in
> > > fdt_rw_probe_() below. So it does seem to be wrong.
> >
> > Ah, right.  So, that's true for all cases, except the call in
> > fdt_open_into().  The idea is that you have to call fdt_open_into()
> > before any of the other rw functions.  fdt_open_into() will re-order
> > the blocks, if necessary, subsequent functions then require them in
> > the "standard" order.
> >
> > So for the cases other than open_into() you can probably put this
> > under VALID_INPUT, I think it's reasonable to include "you've called
> > prerequisite functions correctly" under requiring suitable input.
> >
> > You almost certainly also want to elide the block reordering code in
> > fdt_open_into(), though.  That one probably wants another flag
> 
> OK, v6.
> 
> >
> > > > >       return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> > > > >               || (fdt_off_dt_struct(fdt) <
> > > > >                   (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > > > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > > > >
> > > > >  static int fdt_rw_probe_(void *fdt)
> > > > >  {
> > > > > +     if (can_assume(VALID_DTB))
> > > > > +             return 0;
> > > > >       FDT_RO_PROBE(fdt);
> > > > >
> > > > >       if (fdt_version(fdt) < 17)
> > > > > @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
> > > > >  #define FDT_RW_PROBE(fdt) \
> > > > >       { \
> > > > >               int err_; \
> > > > > -             if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > > > > +             if (!can_assume(VALID_DTB) && \
> > > > > +                 (err_ = fdt_rw_probe_(fdt)) != 0) \
> > > >
> > > >
> > > > You shouldn't need the test both inside fdt_rw_probe_() and in the
> > > > FDT_RW_PROBE() macro, no?
> > >
> > > OK. It saves a small amount of code space but we can look at it once
> > > we get something applied...
> >
> > Ok, do you know why that is?
> 
> Well just that if fdt_rw_probe_() is called it consumes a few bytes.
> If it is never called it won't be included in the SPL image.

That's surprising to me.  fdt_rw_probe_() is local, and with the
assume flag on it becomes essentially a no-op.  So I would have
expected the compiler to inline it to nothing.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 4/7] libfdt: Add support for disabling sanity checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8108 bytes --]

On Tue, Feb 11, 2020 at 01:08:42PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Mon, 10 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Wed, Feb 05, 2020 at 10:40:31PM -0700, Simon Glass wrote:
> > > Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
> > > tree and the parameters to libfdt. This assumption covers that cases where
> > > the problem could be with either.
> > >
> > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > ---
> > >
> > > Changes in v5:
> > > - Include just VALID_INPUT checks in this patch
> > >
> > > Changes in v4: None
> > > Changes in v3: None
> > > Changes in v2: None
> > >
> > >  libfdt/fdt.c    | 12 +++++----
> > >  libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
> > >  2 files changed, 54 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > index 03f2b7d..e2c1da0 100644
> > > --- a/libfdt/fdt.c
> > > +++ b/libfdt/fdt.c
> > > @@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> > >  {
> > >       unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> > >
> > > -     if ((absoffset < offset)
> > > -         || ((absoffset + len) < absoffset)
> > > -         || (absoffset + len) > fdt_totalsize(fdt))
> > > -             return NULL;
> > > +     if (!can_assume(VALID_INPUT))
> > > +             if ((absoffset < offset)
> > > +                 || ((absoffset + len) < absoffset)
> > > +                 || (absoffset + len) > fdt_totalsize(fdt))
> > > +                     return NULL;
> > >
> > >       if (fdt_version(fdt) >= 0x11)
> > >               if (((offset + len) < offset)
> > > @@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > >               return FDT_END;
> > >       }
> > >
> > > -     if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > +     if (!can_assume(VALID_INPUT) &&
> > > +         !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > >               return FDT_END; /* premature end */
> > >
> > >       *nextoffset = FDT_TAGALIGN(offset);
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index b41083f..07c13c9 100644
> > > --- a/libfdt/fdt_ro.c
> > > +++ b/libfdt/fdt_ro.c
> > > @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
> > >       int olen;
> > >       const char *p = fdt_get_name(fdt, offset, &olen);
> > >
> > > -     if (!p || olen < len)
> > > +     if (!p || (!can_assume(VALID_INPUT) && olen < len))
> >
> > Oof, this one's subtle.  We certainly *can* have olen < len even in
> > perfectly valid cases.  However, if we're assuming validly \0
> > terminated strings in the strings block *and* no \0s in s (which you
> > can with assume(VALID_INPUT)), then the memcmp() will necessarily pick
> > up that case.
> >
> > If we also assume memcmp() is the obvious byte-by-byte implementation
> > then it will stop before accessing beyond the end of the strings block
> > string.  But... I don't think that's necessarily the case for all C
> > libraries / runtimes.  So, if this is close to the end of the strings
> > block, the memcmp() could access beyond the dtb buffer, which is a no
> > no.
> >
> > Now, I guess we could have an assume(DUMB_MEMCMP) and/or
> > assume(READ_ACCESS_SLIGHTLY_BEYOND_THE_DTB_WILL_WORK), but we're
> > getting really esoteric now.
> >
> > Is avoiding this one comparison worth it?
> 
> For further context see this commit:
> 
> f1879e1 Add limited read-only support for older (V2 and V3) device
> tree to libfdt.
> 
> Before that we assumed that it was safe to do the memcmp(), so I
> figured we could revert the behaviour with this assumption.

We did, but I'm pretty sure that assumption was wrong.  I think we've
had Coverity complain about a similar construct at some point.

> Another point is that fdt_subnode_offset_namelen() should probably not
> allow namelen to be less than strlen(name). Should we add a comment
> and check for that?

Absolutely not.  namelen is allowed to be less that strlen(name) and
expected to in many common cases.  In general the _namelen() variants
aren't (primarily) about an optimization to avoid a strlen() call.

Rather, they are to allow callers to use these interfaces with a piece
of a larger \0 terminated string without having to either mangle their
longer string in place, or copy sections out.

For example fdt_path_offset() will use namelen < strlen all the time,
because it repeatedly calls subnode_offset_namelen() on individual
path components without copying them out of the path.  Copying them
out would a) be expensive and b) without an allocator would require an
arbitrary length limit.

> Also I don't think I fully understand fdt_nodename_eq_(). It doesn't
> have a function comment so it's not really clear what it is supposed
> to do. But if I call it with:
> 
> fdt_nodename_eq_(fdt, offset, s="ernie", len=5)
> 
> and say that fdt_get_name() returns "fred" with olen=4.
> 
> Now olen < len is true, so this function will return 0, indicating a
> match. But there is no match. What am I missing?

1 is a match, not 0.  Think of it as returning a boolean (that's why
the name is 'eq', not 'cmp').

> Anyway I agree it doesn't seem worth it, but I'd like to get some
> comments into these functions.
> 
> >
> > >               /* short match */
> > >               return 0;
> > >
> 
> [..]
> 
> > > @@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > >       if (!can_assume(VALID_DTB)) {
> > >               if ((err = fdt_ro_probe_(fdt)) < 0)
> > >                       goto fail;
> > > -             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > -                     goto fail;
> > > +             if (can_assume(VALID_INPUT) &&
> >
> > That should be !can_assume, no?
> 
> Yes, although I've dropped it in v6 since the check is now in
> fdt_check_node_offset_().
> >
> > > +                 (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > +             goto fail;
> > >       }
> > >
> > >       nameptr = nh->name;
> > > @@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
> > >       int err;
> > >       const struct fdt_property *prop;
> > >
> > > -     if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > +     if (!can_assume(VALID_INPUT) &&
> > > +         (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > >               if (lenp)
> > >                       *lenp = err;
> > >               return NULL;
> > > @@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
> > >            (offset = fdt_next_property_offset(fdt, offset))) {
> > >               const struct fdt_property *prop;
> > >
> > > -             if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> > > +             prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> > > +             if (!can_assume(VALID_INPUT) && !prop) {
> > >                       offset = -FDT_ERR_INTERNAL;
> >
> > So, arguably you could put this one under a weaker assumption flag.
> > Basicaly FDT_ERR_INTERNAL errors should *never* happen, even with bad
> > input - they're basically assert()s, except I didn't want to rely on
> > the runtime things that assert() needs.
> 
> Yes I see. The fdt_first/next_property_offset() functions should never
> return anything invalid.

Right.  Actually, I guess this could happen if something is
concurrently modifying the fdt blob, but really if that's happening
all bets are off - there are some limits to how safe I care to be :).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/7] libfdt: Add support for disabling dtb checks

[Simon Glass]

Hi David,

On Tue, 11 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Feb 11, 2020 at 01:08:41PM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Mon, 10 Feb 2020 at 18:43, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Mon, Feb 10, 2020 at 06:04:47PM -0700, Simon Glass wrote:
> > > > Hi David,
> > > >
> > > > On Sun, 9 Feb 2020 at 23:31, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > > > >
> > > > > On Wed, Feb 05, 2020 at 10:40:30PM -0700, Simon Glass wrote:
> > > > > > Support ASSUME_VALID_DTB to disable some sanity checks
> > > > > >
> > > > > > If we assume that the DTB itself is valid then we can skip some checks and
> > > > > > save code space. Add various conditions to handle this.
> > > > > >
> > > > > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > > > ---
> > > > > >
> > > > > > Changes in v5:
> > > > > > - Split out VALID_DTB checks into a separate patch
> > > > > >
> > > > > > Changes in v4: None
> > > > > > Changes in v3: None
> > > > > > Changes in v2: None
> > > > > >
> > > > > >  libfdt/fdt.c             | 50 ++++++++++++++++++++++------------------
> > > > > >  libfdt/fdt_ro.c          |  7 ++++--
> > > > > >  libfdt/fdt_rw.c          |  7 +++++-
> > > > > >  libfdt/fdt_sw.c          | 30 ++++++++++++++++--------
> > > > > >  libfdt/libfdt_internal.h |  7 ++++--
> > > > > >  5 files changed, 64 insertions(+), 37 deletions(-)
> > > > > >
> > > > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > > > > index 3e37a4b..03f2b7d 100644
> > > > > > --- a/libfdt/fdt.c
> > > > > > +++ b/libfdt/fdt.c
> > > > > > @@ -81,38 +81,44 @@ int fdt_check_header(const void *fdt)
> > > > > >
> > > > > >       if (fdt_magic(fdt) != FDT_MAGIC)
> > > > > >               return -FDT_ERR_BADMAGIC;
> > > > > > -     hdrsize = fdt_header_size(fdt);
> > > > > >       if ((fdt_version(fdt) < FDT_FIRST_SUPPORTED_VERSION)
> > > > > >           || (fdt_last_comp_version(fdt) > FDT_LAST_SUPPORTED_VERSION))
> > > > > >               return -FDT_ERR_BADVERSION;
> > > > > >       if (fdt_version(fdt) < fdt_last_comp_version(fdt))
> > > > > >               return -FDT_ERR_BADVERSION;
> > > > > > +     hdrsize = fdt_header_size(fdt);
> > > > > > +     if (!can_assume(VALID_DTB)) {
> > > > > >
> > > > > > -     if ((fdt_totalsize(fdt) < hdrsize)
> > > > > > -         || (fdt_totalsize(fdt) > INT_MAX))
> > > > > > -             return -FDT_ERR_TRUNCATED;
> > > > > > -
> > > > > > -     /* Bounds check memrsv block */
> > > > > > -     if (!check_off_(hdrsize, fdt_totalsize(fdt), fdt_off_mem_rsvmap(fdt)))
> > > > > > -             return -FDT_ERR_TRUNCATED;
> > > > > > +             if ((fdt_totalsize(fdt) < hdrsize)
> > > > > > +                 || (fdt_totalsize(fdt) > INT_MAX))
> > > > > > +                     return -FDT_ERR_TRUNCATED;
> > > > > >
> > > > > > -     /* Bounds check structure block */
> > > > > > -     if (fdt_version(fdt) < 17) {
> > > > > > +             /* Bounds check memrsv block */
> > > > > >               if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > > > > -                             fdt_off_dt_struct(fdt)))
> > > > > > +                             fdt_off_mem_rsvmap(fdt)))
> > > > > >                       return -FDT_ERR_TRUNCATED;
> > > > > > -     } else {
> > > > > > +     }
> > > > > > +
> > > > > > +     if (!can_assume(VALID_DTB)) {
> > > > > > +             /* Bounds check structure block */
> > > > > > +             if (fdt_version(fdt) < 17) {
> > > > > > +                     if (!check_off_(hdrsize, fdt_totalsize(fdt),
> > > > > > +                                     fdt_off_dt_struct(fdt)))
> > > > > > +                             return -FDT_ERR_TRUNCATED;
> > > > > > +             } else {
> > > > > > +                     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > > > +                                       fdt_off_dt_struct(fdt),
> > > > > > +                                       fdt_size_dt_struct(fdt)))
> > > > > > +                             return -FDT_ERR_TRUNCATED;
> > > > > > +             }
> > > > > > +
> > > > > > +             /* Bounds check strings block */
> > > > > >               if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > > > -                               fdt_off_dt_struct(fdt),
> > > > > > -                               fdt_size_dt_struct(fdt)))
> > > > > > +                               fdt_off_dt_strings(fdt),
> > > > > > +                               fdt_size_dt_strings(fdt)))
> > > > > >                       return -FDT_ERR_TRUNCATED;
> > > > > >       }
> > > > > >
> > > > > > -     /* Bounds check strings block */
> > > > > > -     if (!check_block_(hdrsize, fdt_totalsize(fdt),
> > > > > > -                       fdt_off_dt_strings(fdt), fdt_size_dt_strings(fdt)))
> > > > > > -             return -FDT_ERR_TRUNCATED;
> > > > > > -
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > > > > @@ -142,7 +148,7 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > > > >
> > > > > >       *nextoffset = -FDT_ERR_TRUNCATED;
> > > > > >       tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > > > > -     if (!tagp)
> > > > > > +     if (!can_assume(VALID_DTB) && !tagp)
> > > > > >               return FDT_END; /* premature end */
> > > > > >       tag = fdt32_to_cpu(*tagp);
> > > > > >       offset += FDT_TAGSIZE;
> > > > > > @@ -154,13 +160,13 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > > > >               do {
> > > > > >                       p = fdt_offset_ptr(fdt, offset++, 1);
> > > > > >               } while (p && (*p != '\0'));
> > > > > > -             if (!p)
> > > > > > +             if (!can_assume(VALID_DTB) && !p)
> > > > > >                       return FDT_END; /* premature end */
> > > > > >               break;
> > > > > >
> > > > > >       case FDT_PROP:
> > > > > >               lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp));
> > > > > > -             if (!lenp)
> > > > > > +             if (!can_assume(VALID_DTB) && !lenp)
> > > > > >                       return FDT_END; /* premature end */
> > > > > >               /* skip-name offset, length and value */
> > > > > >               offset += sizeof(struct fdt_property) - FDT_TAGSIZE
> > > > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > > > index a5c2797..b41083f 100644
> > > > > > --- a/libfdt/fdt_ro.c
> > > > > > +++ b/libfdt/fdt_ro.c
> > > > > > @@ -289,9 +289,12 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > > > >       const char *nameptr;
> > > > > >       int err;
> > > > > >
> > > > > > -     if (((err = fdt_ro_probe_(fdt)) < 0)
> > > > > > -         || ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> > > > > > +     if (!can_assume(VALID_DTB)) {
> > > > > > +             if ((err = fdt_ro_probe_(fdt)) < 0)
> > > > >
> > > > > Seems like it might be cleaner to include the can_assume() check into
> > > > > fdt_ro_probe_().
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > >                       goto fail;
> > > > > > +             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > > > +                     goto fail;
> > > > > > +     }
> > > > > >
> > > > > >       nameptr = nh->name;
> > > > > >
> > > > > > diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> > > > > > index 8795947..7a41a31 100644
> > > > > > --- a/libfdt/fdt_rw.c
> > > > > > +++ b/libfdt/fdt_rw.c
> > > > > > @@ -13,6 +13,8 @@
> > > > > >  static int fdt_blocks_misordered_(const void *fdt,
> > > > > >                                 int mem_rsv_size, int struct_size)
> > > > > >  {
> > > > > > +     if (can_assume(VALID_DTB))
> > > > > > +             return false;
> > > > >
> > > > > Urgh... the spec doesn't actually specify a block order, so I'm not
> > > > > sure we can put this one under VALID_DTB.
> > > >
> > > > But if this returns true then we return -FDT_ERR_BADLAYOUT in
> > > > fdt_rw_probe_() below. So it does seem to be wrong.
> > >
> > > Ah, right.  So, that's true for all cases, except the call in
> > > fdt_open_into().  The idea is that you have to call fdt_open_into()
> > > before any of the other rw functions.  fdt_open_into() will re-order
> > > the blocks, if necessary, subsequent functions then require them in
> > > the "standard" order.
> > >
> > > So for the cases other than open_into() you can probably put this
> > > under VALID_INPUT, I think it's reasonable to include "you've called
> > > prerequisite functions correctly" under requiring suitable input.
> > >
> > > You almost certainly also want to elide the block reordering code in
> > > fdt_open_into(), though.  That one probably wants another flag
> >
> > OK, v6.
> >
> > >
> > > > > >       return (fdt_off_mem_rsvmap(fdt) < FDT_ALIGN(sizeof(struct fdt_header), 8))
> > > > > >               || (fdt_off_dt_struct(fdt) <
> > > > > >                   (fdt_off_mem_rsvmap(fdt) + mem_rsv_size))
> > > > > > @@ -24,6 +26,8 @@ static int fdt_blocks_misordered_(const void *fdt,
> > > > > >
> > > > > >  static int fdt_rw_probe_(void *fdt)
> > > > > >  {
> > > > > > +     if (can_assume(VALID_DTB))
> > > > > > +             return 0;
> > > > > >       FDT_RO_PROBE(fdt);
> > > > > >
> > > > > >       if (fdt_version(fdt) < 17)
> > > > > > @@ -40,7 +44,8 @@ static int fdt_rw_probe_(void *fdt)
> > > > > >  #define FDT_RW_PROBE(fdt) \
> > > > > >       { \
> > > > > >               int err_; \
> > > > > > -             if ((err_ = fdt_rw_probe_(fdt)) != 0) \
> > > > > > +             if (!can_assume(VALID_DTB) && \
> > > > > > +                 (err_ = fdt_rw_probe_(fdt)) != 0) \
> > > > >
> > > > >
> > > > > You shouldn't need the test both inside fdt_rw_probe_() and in the
> > > > > FDT_RW_PROBE() macro, no?
> > > >
> > > > OK. It saves a small amount of code space but we can look at it once
> > > > we get something applied...
> > >
> > > Ok, do you know why that is?
> >
> > Well just that if fdt_rw_probe_() is called it consumes a few bytes.
> > If it is never called it won't be included in the SPL image.
>
> That's surprising to me.  fdt_rw_probe_() is local, and with the
> assume flag on it becomes essentially a no-op.  So I would have
> expected the compiler to inline it to nothing.

Ah yes I was thinking of fdt_ro_probe_(). Well, once we get something
in I'll look at things in detail again and see what can be improved
without too much pain.

Regards,
Simon

Re: [PATCH v5 4/7] libfdt: Add support for disabling sanity checks

[Simon Glass]

Hi David,

On Tue, 11 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
>
> On Tue, Feb 11, 2020 at 01:08:42PM -0700, Simon Glass wrote:
> > Hi David,
> >
> > On Mon, 10 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > >
> > > On Wed, Feb 05, 2020 at 10:40:31PM -0700, Simon Glass wrote:
> > > > Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
> > > > tree and the parameters to libfdt. This assumption covers that cases where
> > > > the problem could be with either.
> > > >
> > > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > ---
> > > >
> > > > Changes in v5:
> > > > - Include just VALID_INPUT checks in this patch
> > > >
> > > > Changes in v4: None
> > > > Changes in v3: None
> > > > Changes in v2: None
> > > >
> > > >  libfdt/fdt.c    | 12 +++++----
> > > >  libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
> > > >  2 files changed, 54 insertions(+), 29 deletions(-)
> > > >
> > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > > index 03f2b7d..e2c1da0 100644
> > > > --- a/libfdt/fdt.c
> > > > +++ b/libfdt/fdt.c
> > > > @@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> > > >  {
> > > >       unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> > > >
> > > > -     if ((absoffset < offset)
> > > > -         || ((absoffset + len) < absoffset)
> > > > -         || (absoffset + len) > fdt_totalsize(fdt))
> > > > -             return NULL;
> > > > +     if (!can_assume(VALID_INPUT))
> > > > +             if ((absoffset < offset)
> > > > +                 || ((absoffset + len) < absoffset)
> > > > +                 || (absoffset + len) > fdt_totalsize(fdt))
> > > > +                     return NULL;
> > > >
> > > >       if (fdt_version(fdt) >= 0x11)
> > > >               if (((offset + len) < offset)
> > > > @@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > >               return FDT_END;
> > > >       }
> > > >
> > > > -     if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > > +     if (!can_assume(VALID_INPUT) &&
> > > > +         !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > >               return FDT_END; /* premature end */
> > > >
> > > >       *nextoffset = FDT_TAGALIGN(offset);
> > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > index b41083f..07c13c9 100644
> > > > --- a/libfdt/fdt_ro.c
> > > > +++ b/libfdt/fdt_ro.c
> > > > @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
> > > >       int olen;
> > > >       const char *p = fdt_get_name(fdt, offset, &olen);
> > > >
> > > > -     if (!p || olen < len)
> > > > +     if (!p || (!can_assume(VALID_INPUT) && olen < len))
> > >
> > > Oof, this one's subtle.  We certainly *can* have olen < len even in
> > > perfectly valid cases.  However, if we're assuming validly \0
> > > terminated strings in the strings block *and* no \0s in s (which you
> > > can with assume(VALID_INPUT)), then the memcmp() will necessarily pick
> > > up that case.
> > >
> > > If we also assume memcmp() is the obvious byte-by-byte implementation
> > > then it will stop before accessing beyond the end of the strings block
> > > string.  But... I don't think that's necessarily the case for all C
> > > libraries / runtimes.  So, if this is close to the end of the strings
> > > block, the memcmp() could access beyond the dtb buffer, which is a no
> > > no.
> > >
> > > Now, I guess we could have an assume(DUMB_MEMCMP) and/or
> > > assume(READ_ACCESS_SLIGHTLY_BEYOND_THE_DTB_WILL_WORK), but we're
> > > getting really esoteric now.
> > >
> > > Is avoiding this one comparison worth it?
> >
> > For further context see this commit:
> >
> > f1879e1 Add limited read-only support for older (V2 and V3) device
> > tree to libfdt.
> >
> > Before that we assumed that it was safe to do the memcmp(), so I
> > figured we could revert the behaviour with this assumption.
>
> We did, but I'm pretty sure that assumption was wrong.  I think we've
> had Coverity complain about a similar construct at some point.

OK.

>
> > Another point is that fdt_subnode_offset_namelen() should probably not
> > allow namelen to be less than strlen(name). Should we add a comment
> > and check for that?
>
> Absolutely not.  namelen is allowed to be less that strlen(name) and
> expected to in many common cases.  In general the _namelen() variants
> aren't (primarily) about an optimization to avoid a strlen() call.
>
> Rather, they are to allow callers to use these interfaces with a piece
> of a larger \0 terminated string without having to either mangle their
> longer string in place, or copy sections out.
>
> For example fdt_path_offset() will use namelen < strlen all the time,
> because it repeatedly calls subnode_offset_namelen() on individual
> path components without copying them out of the path.  Copying them
> out would a) be expensive and b) without an allocator would require an
> arbitrary length limit.

OK I had it around the wrong way. So what does the comment 'short match' mean?

>
> > Also I don't think I fully understand fdt_nodename_eq_(). It doesn't
> > have a function comment so it's not really clear what it is supposed
> > to do. But if I call it with:
> >
> > fdt_nodename_eq_(fdt, offset, s="ernie", len=5)
> >
> > and say that fdt_get_name() returns "fred" with olen=4.
> >
> > Now olen < len is true, so this function will return 0, indicating a
> > match. But there is no match. What am I missing?
>
> 1 is a match, not 0.  Think of it as returning a boolean (that's why
> the name is 'eq', not 'cmp').

OK. I wonder if we could use stdbool?

>
> > Anyway I agree it doesn't seem worth it, but I'd like to get some
> > comments into these functions.
> >
> > >
> > > >               /* short match */
> > > >               return 0;
> > > >
> >
> > [..]
> >
> > > > @@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > >       if (!can_assume(VALID_DTB)) {
> > > >               if ((err = fdt_ro_probe_(fdt)) < 0)
> > > >                       goto fail;
> > > > -             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > -                     goto fail;
> > > > +             if (can_assume(VALID_INPUT) &&
> > >
> > > That should be !can_assume, no?
> >
> > Yes, although I've dropped it in v6 since the check is now in
> > fdt_check_node_offset_().
> > >
> > > > +                 (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > +             goto fail;
> > > >       }
> > > >
> > > >       nameptr = nh->name;
> > > > @@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
> > > >       int err;
> > > >       const struct fdt_property *prop;
> > > >
> > > > -     if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > > +     if (!can_assume(VALID_INPUT) &&
> > > > +         (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > >               if (lenp)
> > > >                       *lenp = err;
> > > >               return NULL;
> > > > @@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
> > > >            (offset = fdt_next_property_offset(fdt, offset))) {
> > > >               const struct fdt_property *prop;
> > > >
> > > > -             if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> > > > +             prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> > > > +             if (!can_assume(VALID_INPUT) && !prop) {
> > > >                       offset = -FDT_ERR_INTERNAL;
> > >
> > > So, arguably you could put this one under a weaker assumption flag.
> > > Basicaly FDT_ERR_INTERNAL errors should *never* happen, even with bad
> > > input - they're basically assert()s, except I didn't want to rely on
> > > the runtime things that assert() needs.
> >
> > Yes I see. The fdt_first/next_property_offset() functions should never
> > return anything invalid.
>
> Right.  Actually, I guess this could happen if something is
> concurrently modifying the fdt blob, but really if that's happening
> all bets are off - there are some limits to how safe I care to be :).

Indeed.

Regards,
Simon

Re: [PATCH v5 4/7] libfdt: Add support for disabling sanity checks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 9631 bytes --]

On Tue, Feb 11, 2020 at 06:23:46PM -0700, Simon Glass wrote:
> Hi David,
> 
> On Tue, 11 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >
> > On Tue, Feb 11, 2020 at 01:08:42PM -0700, Simon Glass wrote:
> > > Hi David,
> > >
> > > On Mon, 10 Feb 2020 at 18:12, David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6rL0yjEaa7Ru@public.gmane.orgau> wrote:
> > > >
> > > > On Wed, Feb 05, 2020 at 10:40:31PM -0700, Simon Glass wrote:
> > > > > Allow enabling ASSUME_VALID_INPUT to disable sanity checks on the device
> > > > > tree and the parameters to libfdt. This assumption covers that cases where
> > > > > the problem could be with either.
> > > > >
> > > > > Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > > > ---
> > > > >
> > > > > Changes in v5:
> > > > > - Include just VALID_INPUT checks in this patch
> > > > >
> > > > > Changes in v4: None
> > > > > Changes in v3: None
> > > > > Changes in v2: None
> > > > >
> > > > >  libfdt/fdt.c    | 12 +++++----
> > > > >  libfdt/fdt_ro.c | 71 ++++++++++++++++++++++++++++++++-----------------
> > > > >  2 files changed, 54 insertions(+), 29 deletions(-)
> > > > >
> > > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > > > > index 03f2b7d..e2c1da0 100644
> > > > > --- a/libfdt/fdt.c
> > > > > +++ b/libfdt/fdt.c
> > > > > @@ -126,10 +126,11 @@ const void *fdt_offset_ptr(const void *fdt, int offset, unsigned int len)
> > > > >  {
> > > > >       unsigned absoffset = offset + fdt_off_dt_struct(fdt);
> > > > >
> > > > > -     if ((absoffset < offset)
> > > > > -         || ((absoffset + len) < absoffset)
> > > > > -         || (absoffset + len) > fdt_totalsize(fdt))
> > > > > -             return NULL;
> > > > > +     if (!can_assume(VALID_INPUT))
> > > > > +             if ((absoffset < offset)
> > > > > +                 || ((absoffset + len) < absoffset)
> > > > > +                 || (absoffset + len) > fdt_totalsize(fdt))
> > > > > +                     return NULL;
> > > > >
> > > > >       if (fdt_version(fdt) >= 0x11)
> > > > >               if (((offset + len) < offset)
> > > > > @@ -185,7 +186,8 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset)
> > > > >               return FDT_END;
> > > > >       }
> > > > >
> > > > > -     if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > > > +     if (!can_assume(VALID_INPUT) &&
> > > > > +         !fdt_offset_ptr(fdt, startoffset, offset - startoffset))
> > > > >               return FDT_END; /* premature end */
> > > > >
> > > > >       *nextoffset = FDT_TAGALIGN(offset);
> > > > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > > > index b41083f..07c13c9 100644
> > > > > --- a/libfdt/fdt_ro.c
> > > > > +++ b/libfdt/fdt_ro.c
> > > > > @@ -16,7 +16,7 @@ static int fdt_nodename_eq_(const void *fdt, int offset,
> > > > >       int olen;
> > > > >       const char *p = fdt_get_name(fdt, offset, &olen);
> > > > >
> > > > > -     if (!p || olen < len)
> > > > > +     if (!p || (!can_assume(VALID_INPUT) && olen < len))
> > > >
> > > > Oof, this one's subtle.  We certainly *can* have olen < len even in
> > > > perfectly valid cases.  However, if we're assuming validly \0
> > > > terminated strings in the strings block *and* no \0s in s (which you
> > > > can with assume(VALID_INPUT)), then the memcmp() will necessarily pick
> > > > up that case.
> > > >
> > > > If we also assume memcmp() is the obvious byte-by-byte implementation
> > > > then it will stop before accessing beyond the end of the strings block
> > > > string.  But... I don't think that's necessarily the case for all C
> > > > libraries / runtimes.  So, if this is close to the end of the strings
> > > > block, the memcmp() could access beyond the dtb buffer, which is a no
> > > > no.
> > > >
> > > > Now, I guess we could have an assume(DUMB_MEMCMP) and/or
> > > > assume(READ_ACCESS_SLIGHTLY_BEYOND_THE_DTB_WILL_WORK), but we're
> > > > getting really esoteric now.
> > > >
> > > > Is avoiding this one comparison worth it?
> > >
> > > For further context see this commit:
> > >
> > > f1879e1 Add limited read-only support for older (V2 and V3) device
> > > tree to libfdt.
> > >
> > > Before that we assumed that it was safe to do the memcmp(), so I
> > > figured we could revert the behaviour with this assumption.
> >
> > We did, but I'm pretty sure that assumption was wrong.  I think we've
> > had Coverity complain about a similar construct at some point.
> 
> OK.
> 
> >
> > > Another point is that fdt_subnode_offset_namelen() should probably not
> > > allow namelen to be less than strlen(name). Should we add a comment
> > > and check for that?
> >
> > Absolutely not.  namelen is allowed to be less that strlen(name) and
> > expected to in many common cases.  In general the _namelen() variants
> > aren't (primarily) about an optimization to avoid a strlen() call.
> >
> > Rather, they are to allow callers to use these interfaces with a piece
> > of a larger \0 terminated string without having to either mangle their
> > longer string in place, or copy sections out.
> >
> > For example fdt_path_offset() will use namelen < strlen all the time,
> > because it repeatedly calls subnode_offset_namelen() on individual
> > path components without copying them out of the path.  Copying them
> > out would a) be expensive and b) without an allocator would require an
> > arbitrary length limit.
> 
> OK I had it around the wrong way. So what does the comment 'short match' mean?

I guess it means "the string in the strings block is shorter than the
given string, and therefore doesn't match".  We should probably just
drop the comment, it's not very illuminating.

> > > Also I don't think I fully understand fdt_nodename_eq_(). It doesn't
> > > have a function comment so it's not really clear what it is supposed
> > > to do. But if I call it with:
> > >
> > > fdt_nodename_eq_(fdt, offset, s="ernie", len=5)
> > >
> > > and say that fdt_get_name() returns "fred" with olen=4.
> > >
> > > Now olen < len is true, so this function will return 0, indicating a
> > > match. But there is no match. What am I missing?
> >
> > 1 is a match, not 0.  Think of it as returning a boolean (that's why
> > the name is 'eq', not 'cmp').
> 
> OK. I wonder if we could use stdbool?

Yeah, maybe.  I try to be conservative with what C features I use in
libfdt, for the a sake bootloader environments built with weird old
toolchains.  I don't really have a sense of how widely/long stdbool
has been implemented.

> > > Anyway I agree it doesn't seem worth it, but I'd like to get some
> > > comments into these functions.
> > >
> > > >
> > > > >               /* short match */
> > > > >               return 0;
> > > > >
> > >
> > > [..]
> > >
> > > > > @@ -292,8 +304,9 @@ const char *fdt_get_name(const void *fdt, int nodeoffset, int *len)
> > > > >       if (!can_assume(VALID_DTB)) {
> > > > >               if ((err = fdt_ro_probe_(fdt)) < 0)
> > > > >                       goto fail;
> > > > > -             if ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > > -                     goto fail;
> > > > > +             if (can_assume(VALID_INPUT) &&
> > > >
> > > > That should be !can_assume, no?
> > >
> > > Yes, although I've dropped it in v6 since the check is now in
> > > fdt_check_node_offset_().
> > > >
> > > > > +                 (err = fdt_check_node_offset_(fdt, nodeoffset)) < 0)
> > > > > +             goto fail;
> > > > >       }
> > > > >
> > > > >       nameptr = nh->name;
> > > > > @@ -349,7 +362,8 @@ static const struct fdt_property *fdt_get_property_by_offset_(const void *fdt,
> > > > >       int err;
> > > > >       const struct fdt_property *prop;
> > > > >
> > > > > -     if ((err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > > > +     if (!can_assume(VALID_INPUT) &&
> > > > > +         (err = fdt_check_prop_offset_(fdt, offset)) < 0) {
> > > > >               if (lenp)
> > > > >                       *lenp = err;
> > > > >               return NULL;
> > > > > @@ -391,7 +405,8 @@ static const struct fdt_property *fdt_get_property_namelen_(const void *fdt,
> > > > >            (offset = fdt_next_property_offset(fdt, offset))) {
> > > > >               const struct fdt_property *prop;
> > > > >
> > > > > -             if (!(prop = fdt_get_property_by_offset_(fdt, offset, lenp))) {
> > > > > +             prop = fdt_get_property_by_offset_(fdt, offset, lenp);
> > > > > +             if (!can_assume(VALID_INPUT) && !prop) {
> > > > >                       offset = -FDT_ERR_INTERNAL;
> > > >
> > > > So, arguably you could put this one under a weaker assumption flag.
> > > > Basicaly FDT_ERR_INTERNAL errors should *never* happen, even with bad
> > > > input - they're basically assert()s, except I didn't want to rely on
> > > > the runtime things that assert() needs.
> > >
> > > Yes I see. The fdt_first/next_property_offset() functions should never
> > > return anything invalid.
> >
> > Right.  Actually, I guess this could happen if something is
> > concurrently modifying the fdt blob, but really if that's happening
> > all bets are off - there are some limits to how safe I care to be :).
> 
> Indeed.
> 
> Regards,
> Simon
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]