[RFC v2 0/2] Prevent alignment issues with "in place" FIT configurations

[RFC v2 0/2] Prevent alignment issues with "in place" FIT configurations

[Bjørn Mork]

Looking for some feedback on whether this is a problem wanting
to be solved, and if so, whether this solution would be acceptible
(after some polishing).

I recently hit alignment issues on a device where U-Boot has been
modified to force "in place" fdt loading regardless of the fdt_high
environment variable.  I realize that this is not a U-Boot problem,
but I still think that it would be worthwile to make mkimage produce
FIT images suitable for such devices.

This is only one of many possible solutions.  The libfdt changes will
obviously have to be accepted by the dtc community.  I'v posted a
similar request to the devicetree-compiler list with those changes.
The libfdt patch is included here for convenience.

The expected impact for U-Boot is only the tools/fit_image.c patch.


Bjørn 


Changes v2:
 - actually use the new fdt_alignprop() instead of an earlier PoC hack


Bjørn Mork (2):
  libfdt: add fdt_alignprop
  mkimage: Align fdt images in FIT to 8 bytes

 scripts/dtc/libfdt/fdt_rw.c          | 26 +++++++++++++++++++
 scripts/dtc/libfdt/fdt_wip.c         |  2 +-
 scripts/dtc/libfdt/libfdt.h          | 30 +++++++++++++++++++++
 scripts/dtc/libfdt/libfdt_internal.h |  1 +
 tools/fit_image.c                    | 39 ++++++++++++++++++++++++++++
 5 files changed, 97 insertions(+), 1 deletion(-)

-- 
2.30.2

[RFC v2 1/2] libfdt: add fdt_alignprop

[Bjørn Mork]

Properties are sometimes used to store data with stricter alignment
requirements than the 4-byte fdt tag alignment. Aligning the offset
of the property data will alloe a client to directly address it,
without reloaction, provided the fdt is loaded on a similarily
aligned boundary.

One known use-case for this is the U-Boot FIT images, which may
embed nested fdt blobs inside properties of the outer FIT fdt.
Although strongly discouraged, U-Boot can be configured to use these
embedded blobs "in place". This is discouraged because if only will
work if the offset of the property value, i.e. the embedded fdt,
happens to match the 8-byte boundary required by the devicetree
spec.

Adding the ability to align property data allows U-Boot tools to
reliably create FIT images for such use.

Other use cases are currently not known, but anticipated.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 scripts/dtc/libfdt/fdt_rw.c          | 26 ++++++++++++++++++++++++
 scripts/dtc/libfdt/fdt_wip.c         |  2 +-
 scripts/dtc/libfdt/libfdt.h          | 30 ++++++++++++++++++++++++++++
 scripts/dtc/libfdt/libfdt_internal.h |  1 +
 4 files changed, 58 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/libfdt/fdt_rw.c b/scripts/dtc/libfdt/fdt_rw.c
index 2eb2b38703be..9e0e8dfeec8c 100644
--- a/scripts/dtc/libfdt/fdt_rw.c
+++ b/scripts/dtc/libfdt/fdt_rw.c
@@ -331,6 +331,32 @@ int fdt_delprop(void *fdt, int nodeoffset, const char *name)
 	return fdt_splice_struct_(fdt, prop, proplen, 0);
 }
 
+int fdt_alignprop(void *fdt, int nodeoffset, const char *name, size_t align)
+{
+	struct fdt_property *prop;
+	int err, len, diff;
+
+	FDT_RW_PROBE(fdt);
+
+	if (align != FDT_TAGALIGN(align))
+		return -FDT_ERR_BADOFFSET;
+
+	prop = fdt_get_property_w(fdt, nodeoffset, name, &len);
+	if (!prop)
+		return len;
+
+	diff = FDT_ALIGN((uintptr_t)prop->data, align) - (uintptr_t)prop->data;
+	if (!diff)
+		return 0;
+
+	err = fdt_splice_struct_(fdt, prop, 0, diff);
+	if (err)
+		return err;
+
+	fdt_nop_region_(prop, diff);
+	return 0;
+}
+
 int fdt_add_subnode_namelen(void *fdt, int parentoffset,
 			    const char *name, int namelen)
 {
diff --git a/scripts/dtc/libfdt/fdt_wip.c b/scripts/dtc/libfdt/fdt_wip.c
index c2d7566a67dc..10a75058f0f0 100644
--- a/scripts/dtc/libfdt/fdt_wip.c
+++ b/scripts/dtc/libfdt/fdt_wip.c
@@ -48,7 +48,7 @@ int fdt_setprop_inplace(void *fdt, int nodeoffset, const char *name,
 						   val, len);
 }
 
-static void fdt_nop_region_(void *start, int len)
+void fdt_nop_region_(void *start, int len)
 {
 	fdt32_t *p;
 
diff --git a/scripts/dtc/libfdt/libfdt.h b/scripts/dtc/libfdt/libfdt.h
index d706f85962bd..4490f9feb7ce 100644
--- a/scripts/dtc/libfdt/libfdt.h
+++ b/scripts/dtc/libfdt/libfdt.h
@@ -1928,6 +1928,36 @@ int fdt_appendprop_addrrange(void *fdt, int parent, int nodeoffset,
  */
 int fdt_delprop(void *fdt, int nodeoffset, const char *name);
 
+/**
+ * fdt_alignprop - align property data to a given boundary
+ * @fdt: pointer to the device tree blob
+ * @nodeoffset: offset of the node whose property to nop
+ * @name: name of the property to nop
+ * @align: requested alignment boundary in bytes (must be multiple of 4)
+ *
+ * fdt_alignprop() will align the property data to a chosen boundary
+ * by injecting nop tags in front of the given property.  The
+ * alignment must be a multiple of 4.
+ *
+ * This function may insert nop tags in the blob, and will therefore
+ * change the offsets of some existing nodes.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_NOSPACE, there is insufficient free space in the blob to
+ *		move the property to the new boundary
+ *	-FDT_ERR_NOTFOUND, node does not have the named property
+ *	-FDT_ERR_BADOFFSET, nodeoffset did not point to FDT_BEGIN_NODE tag
+ *		or the requested alignment was not a multiple of 4
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_alignprop(void *fdt, int nodeoffset, const char *name, size_t align);
+
 /**
  * fdt_add_subnode_namelen - creates a new node based on substring
  * @fdt: pointer to the device tree blob
diff --git a/scripts/dtc/libfdt/libfdt_internal.h b/scripts/dtc/libfdt/libfdt_internal.h
index 5436e2ceeacf..7abee9c21057 100644
--- a/scripts/dtc/libfdt/libfdt_internal.h
+++ b/scripts/dtc/libfdt/libfdt_internal.h
@@ -25,6 +25,7 @@ int fdt_check_node_offset_(const void *fdt, int offset);
 int fdt_check_prop_offset_(const void *fdt, int offset);
 const char *fdt_find_string_(const char *strtab, int tabsize, const char *s);
 int fdt_node_end_offset_(void *fdt, int nodeoffset);
+void fdt_nop_region_(void *start, int len);
 
 static inline const void *fdt_offset_ptr_(const void *fdt, int offset)
 {
-- 
2.30.2

Re: [RFC v2 1/2] libfdt: add fdt_alignprop

[Simon Glass]

Hi Bjørn,

On Sun, 6 Nov 2022 at 11:20, Bjørn Mork <bjorn@mork.no> wrote:
>
> Properties are sometimes used to store data with stricter alignment
> requirements than the 4-byte fdt tag alignment. Aligning the offset
> of the property data will alloe a client to directly address it,
> without reloaction, provided the fdt is loaded on a similarily
> aligned boundary.
>
> One known use-case for this is the U-Boot FIT images, which may
> embed nested fdt blobs inside properties of the outer FIT fdt.
> Although strongly discouraged, U-Boot can be configured to use these
> embedded blobs "in place". This is discouraged because if only will
> work if the offset of the property value, i.e. the embedded fdt,
> happens to match the 8-byte boundary required by the devicetree
> spec.
>
> Adding the ability to align property data allows U-Boot tools to
> reliably create FIT images for such use.
>
> Other use cases are currently not known, but anticipated.
>
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> ---
>  scripts/dtc/libfdt/fdt_rw.c          | 26 ++++++++++++++++++++++++
>  scripts/dtc/libfdt/fdt_wip.c         |  2 +-
>  scripts/dtc/libfdt/libfdt.h          | 30 ++++++++++++++++++++++++++++
>  scripts/dtc/libfdt/libfdt_internal.h |  1 +
>  4 files changed, 58 insertions(+), 1 deletion(-)

Can you use an external FIT?

Regards,
Simon

Re: [RFC v2 1/2] libfdt: add fdt_alignprop

[Bjørn Mork]

Simon Glass <sjg@chromium.org> writes:

> Can you use an external FIT?

Maybe.  I couldn't make that work in a quick test, but that is probably
just me doing something wrong.

But there are other workarounds for this specific device, yes.
Appending the DTB to the kernel image is verified to work.  So it is
possible to create a bootable image.

It's also possible to build a working FIT image with embedded fdt blobs
using existing tools, provided that you are willing to run a few
iterations while adjusting e.g. descriptions.  I just believe it's
unecessarily hard, resulting in complicated build rules when a simple
call to mkimage should have been enough.


Bjørn

[RFC v2 2/2] mkimage: Align fdt images in FIT to 8 bytes

[Bjørn Mork]

"In place" configurations are strongly discouraged, but known to be
used.  This can cause hard to debug alignment problems since the
value of the "data" property only is guaranteed to be 4-byte
aligned.

Unconditionally aligning fdt images to 8 byte boundaries will
prevent these problems, at the maximum price of 4 bytes per fdt.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 tools/fit_image.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/tools/fit_image.c b/tools/fit_image.c
index 923a9755b709..1e060784895d 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -24,6 +24,41 @@
 
 static struct legacy_img_hdr header;
 
+/**
+ * fit_align_fdt_images() - Align all fdt images in the FIT to 8 bytes
+ *
+ * An fdt blob must be placed at an 8 byte aligned offset if it is to
+ * be used "in place". This will not be verified by U-Boot. Simply align
+ * all IH_TYPE_FLATDT images to prevent problems.
+ */
+static int fit_align_fdt_images(void *fit)
+{
+	int images_noffset,  noffset, ret;
+	uint8_t type;
+	size_t size;
+	const void *ptr;
+
+	images_noffset = fdt_path_offset(fit, FIT_IMAGES_PATH);
+	if (images_noffset < 0) {
+		printf("Can't find images parent node '%s' (%s)\n",
+		       FIT_IMAGES_PATH, fdt_strerror(images_noffset));
+		return images_noffset;
+	}
+
+	for (noffset = fdt_first_subnode(fit, images_noffset);
+	     noffset >= 0;
+	     noffset = fdt_next_subnode(fit, noffset)) {
+		fit_image_get_type(fit, noffset, &type);
+		if (type != IH_TYPE_FLATDT)
+			continue;
+
+		ret = fdt_alignprop(fit, noffset, FIT_DATA_PROP, 8);
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
 			     const char *tmpfile)
 {
@@ -81,6 +116,10 @@ static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
 						&params->summary);
 	}
 
+	if (!ret) {
+		ret = fit_align_fdt_images(ptr);
+	}
+
 	if (dest_blob) {
 		munmap(dest_blob, destfd_size);
 		close(destfd);
-- 
2.30.2

Re: [RFC v2 0/2] Prevent alignment issues with "in place" FIT configurations

[Bjørn Mork]

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

So if anyone looked at this, then you've noticed that it fails to
consider signing.

The design makes it hard to support the combination.  Algnment must run
last since signing may inject variable sized nodes before the fdt data
properties.  Signing must run last since it hashes the blob as it is,
inluding FDT_NOP tags and property order.

But we can trick this int working by signing before aligning to create
the signature nodes with their proper size and position, and then
sign again as a final step if we had to inject any FDT_NOP tags.

The attached fix works for me, creating valid signatures with aligned
images no matter how many times I re-sign the FIT with different length
signature comments.

Downsides is the obvious double signing, which we already accept for
resizing, and a build-up of FDT_NOP tags.  The latter is only an issue
if you re-sign with signature node size changes. And there's at most one
tag added per fdt node per signature update, so it's hardly a major
problem.


Bjørn


[-- Attachment #2: 0001-fix-re-sign-if-nops-were-added.patch --]
[-- Type: text/x-diff, Size: 1525 bytes --]

From 00f5cf3b08e44856ed826d427f63743180f3ae90 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@mork.no>
Date: Sun, 6 Nov 2022 22:13:53 +0100
Subject: [RFC v2 3/2] fix: re-sign if nops were added
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 tools/fit_image.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/tools/fit_image.c b/tools/fit_image.c
index 2e215ca2199d..c29e209a8eea 100644
--- a/tools/fit_image.c
+++ b/tools/fit_image.c
@@ -66,6 +66,7 @@ static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
 	struct stat sbuf;
 	void *ptr;
 	int ret = 0;
+	size_t oldsize;
 
 	tfd = mmap_fdt(params->cmdname, tmpfile, size_inc, &ptr, &sbuf, true,
 		       false);
@@ -115,9 +116,22 @@ static int fit_add_file_data(struct image_tool_params *params, size_t size_inc,
 	}
 
 	if (!ret) {
+		oldsize = fdt_size_dt_struct(ptr);
 		ret = fit_align_fdt_images(ptr);
 	}
 
+	/* new FDT_NOP tags must be included in the signed regions */
+	if (!ret && oldsize != fdt_size_dt_struct(ptr)) {
+		ret = fit_add_verification_data(params->keydir,
+						params->keyfile, dest_blob, ptr,
+						params->comment,
+						params->require_keys,
+						params->engine_id,
+						params->cmdname,
+						params->algo_name,
+						&params->summary);
+	}
+
 	if (dest_blob) {
 		munmap(dest_blob, destfd_size);
 		close(destfd);
-- 
2.30.2