[PATCH v3] libfdt: overlay: ensure that existing phandles are not overwritten

* [PATCH v3] libfdt: overlay: ensure that existing phandles are not overwritten
@ 2024-02-25 17:54 Uwe Kleine-König
  2024-02-26  5:53 ` David Gibson
  2024-03-14 10:40 ` David Gibson
  0 siblings, 2 replies; 6+ messages in thread
From: Uwe Kleine-König @ 2024-02-25 17:54 UTC (permalink / raw)
  To: David Gibson; +Cc: Yves-Alexis Perez, devicetree-compiler, entwicklung

A phandle in an overlay is not supposed to overwrite a phandle that
already exists in the base dtb as this breaks references to the
respective node in the base.

So add another iteration over the fdto that checks for such overwrites
and fixes the fdto phandle's value to match the fdt's.

A test is added that checks that newly added phandles and existing
phandles work as expected.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

here comes the next iteration of the patch that fixes overlay
application to not overwrite existing phandles.

It is rebased to current main branch. The changes since v2 are:

 - Add documentation
 - Apply the simplification from 24f60011fd43 ("libfdt: Simplify
   adjustment of values for local fixups") in the functions added here.
 - Rename functions using shorter and better names
 - Changed the test device trees to yield a hole in the phandle space
 - Checked each phandle value not being overwritten separately

Note I didn't switch the order of overlay_prevent_phandle_overwrite() and
overlay_fixup_phandles() because the overlay's phandles must be resolved
before I can do the recursion needed in
overlay_prevent_phandle_overwrite().

Best regards
Uwe

 libfdt/fdt_overlay.c              | 251 ++++++++++++++++++++++++++++++
 tests/overlay_base_phandle.dts    |  21 +++
 tests/overlay_overlay_phandle.dts |  34 ++++
 tests/run_tests.sh                |  28 ++++
 4 files changed, 334 insertions(+)
 create mode 100644 tests/overlay_base_phandle.dts
 create mode 100644 tests/overlay_overlay_phandle.dts

diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
index 7ec839ae7118..f500f2d07030 100644
--- a/libfdt/fdt_overlay.c
+++ b/libfdt/fdt_overlay.c
@@ -498,6 +498,249 @@ static int overlay_fixup_phandles(void *fdt, void *fdto)
 	return 0;
 }
 
+/**
+ * overlay_adjust_local_conflicting_phandle: Changes a phandle value
+ * @fdto: Device tree overlay
+ * @node: The node the phandle is set for
+ * @fdt_phandle: The new value for the phandle
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_adjust_local_conflicting_phandle(void *fdto, int node,
+						    uint32_t fdt_phandle)
+{
+	const fdt32_t *php;
+	int len, ret;
+
+	php = fdt_getprop(fdto, node, "phandle", &len);
+	if (php && len == sizeof(*php)) {
+		ret = fdt_setprop_inplace_u32(fdto, node, "phandle", fdt_phandle);
+		if (ret)
+			return ret;
+	}
+
+	php = fdt_getprop(fdto, node, "linux,phandle", &len);
+	if (php && len == sizeof(*php)) {
+		ret = fdt_setprop_inplace_u32(fdto, node, "linux,phandle", fdt_phandle);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * overlay_update_node_conflicting_references - Recursively replace phandle values
+ * @fdto: Device tree overlay blob
+ * @tree_node: Node to recurse into
+ * @fixup_node: Node offset of the matching local fixups node
+ * @fdt_phandle: Value to replace phandles with
+ * @fdto_phandle: Value to be replaced
+ *
+ * Replaces all phandles with value @fdto_phandle by @fdt_phandle.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_update_node_conflicting_references(void *fdto, int tree_node,
+						      int fixup_node,
+						      uint32_t fdt_phandle,
+						      uint32_t fdto_phandle)
+{
+	int fixup_prop;
+	int fixup_child;
+	int ret;
+
+	fdt_for_each_property_offset(fixup_prop, fdto, fixup_node) {
+		const fdt32_t *fixup_val;
+		const char *name;
+		char *tree_val;
+		int fixup_len;
+		int tree_len;
+		int i;
+
+		fixup_val = fdt_getprop_by_offset(fdto, fixup_prop,
+						  &name, &fixup_len);
+		if (!fixup_val)
+			return fixup_len;
+
+		if (fixup_len % sizeof(uint32_t))
+			return -FDT_ERR_BADOVERLAY;
+		fixup_len /= sizeof(uint32_t);
+
+		tree_val = fdt_getprop_w(fdto, tree_node, name, &tree_len);
+		if (!tree_val) {
+			if (tree_len == -FDT_ERR_NOTFOUND)
+				return -FDT_ERR_BADOVERLAY;
+
+			return tree_len;
+		}
+
+		for (i = 0; i < fixup_len; i++) {
+			fdt32_t *refp;
+			uint32_t valp;
+
+			refp = (fdt32_t *)(tree_val + fdt32_ld_(fixup_val + i));
+			valp = fdt32_ld(refp);
+
+			if (valp == fdto_phandle)
+				fdt32_st(refp, fdt_phandle);
+		}
+	}
+
+	fdt_for_each_subnode(fixup_child, fdto, fixup_node) {
+		const char *fixup_child_name = fdt_get_name(fdto, fixup_child, NULL);
+		int tree_child;
+
+		tree_child = fdt_subnode_offset(fdto, tree_node, fixup_child_name);
+
+		if (tree_child == -FDT_ERR_NOTFOUND)
+			return -FDT_ERR_BADOVERLAY;
+		if (tree_child < 0)
+			return tree_child;
+
+		ret = overlay_update_node_conflicting_references(fdto, tree_child,
+								 fixup_child,
+								 fdt_phandle,
+								 fdto_phandle);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * overlay_update_local_conflicting_references - Recursively replace phandle values
+ * @fdto: Device tree overlay blob
+ * @fdt_phandle: Value to replace phandles with
+ * @fdto_phandle: Value to be replaced
+ *
+ * Replaces all phandles with value @fdto_phandle by @fdt_phandle.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_update_local_conflicting_references(void *fdto,
+						       uint32_t fdt_phandle,
+						       uint32_t fdto_phandle)
+{
+	int fixups;
+
+	fixups = fdt_path_offset(fdto, "/__local_fixups__");
+	if (fixups == -FDT_ERR_NOTFOUND)
+		return 0;
+	if (fixups < 0)
+		return fixups;
+
+	return overlay_update_node_conflicting_references(fdto, 0, fixups,
+							  fdt_phandle,
+							  fdto_phandle);
+}
+
+/**
+ * overlay_prevent_phandle_overwrite_node - Helper function for overlay_prevent_phandle_overwrite
+ * @fdt: Base Device tree blob
+ * @fdtnode: Node in fdt that is checked for an overwrite
+ * @fdto: Device tree overlay blob
+ * @fdtonode: Node in fdto matching @fdtnode
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_prevent_phandle_overwrite_node(void *fdt, int fdtnode,
+						  void *fdto, int fdtonode)
+{
+	uint32_t fdt_phandle, fdto_phandle;
+	int fdtochild;
+
+	fdt_phandle = fdt_get_phandle(fdt, fdtnode);
+	fdto_phandle = fdt_get_phandle(fdto, fdtonode);
+
+	if (fdt_phandle && fdto_phandle) {
+		int ret;
+
+		ret = overlay_adjust_local_conflicting_phandle(fdto, fdtonode,
+							       fdt_phandle);
+		if (ret)
+			return ret;
+
+		ret = overlay_update_local_conflicting_references(fdto,
+								  fdt_phandle,
+								  fdto_phandle);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(fdtochild, fdto, fdtonode) {
+		const char *name = fdt_get_name(fdto, fdtochild, NULL);
+		int fdtchild;
+		int ret;
+
+		fdtchild = fdt_subnode_offset(fdt, fdtnode, name);
+		if (fdtchild == -FDT_ERR_NOTFOUND)
+			/*
+			 * no further overwrites possible here as this node is
+			 * new
+			 */
+			continue;
+
+		ret = overlay_prevent_phandle_overwrite_node(fdt, fdtchild,
+							     fdto, fdtochild);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * overlay_prevent_phandle_overwrite - Fixes overlay phandles to not overwrite base phandles
+ * @fdt: Base Device Tree blob
+ * @fdto: Device tree overlay blob
+ *
+ * Checks recursively if applying fdto overwrites phandle values in the base
+ * dtb. When such a phandle is found, the fdto is changed to use the fdt's
+ * phandle value to not break references in the base.
+ *
+ * returns:
+ *      0 on success
+ *      Negative error code on failure
+ */
+static int overlay_prevent_phandle_overwrite(void *fdt, void *fdto)
+{
+	int fragment;
+
+	fdt_for_each_subnode(fragment, fdto, 0) {
+		int overlay;
+		int target;
+		int ret;
+
+		overlay = fdt_subnode_offset(fdto, fragment, "__overlay__");
+		if (overlay == -FDT_ERR_NOTFOUND)
+			continue;
+
+		if (overlay < 0)
+			return overlay;
+
+		target = fdt_overlay_target_offset(fdt, fdto, fragment, NULL);
+		if (target < 0)
+			return target;
+
+		ret = overlay_prevent_phandle_overwrite_node(fdt, target,
+							     fdto, overlay);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 /**
  * overlay_apply_node - Merges a node into the base device tree
  * @fdt: Base Device Tree blob
@@ -802,18 +1045,26 @@ int fdt_overlay_apply(void *fdt, void *fdto)
 	if (ret)
 		goto err;
 
+	/* Increase all phandles in the fdto by delta */
 	ret = overlay_adjust_local_phandles(fdto, delta);
 	if (ret)
 		goto err;
 
+	/* Adapt the phandle values in fdto to the above increase */
 	ret = overlay_update_local_references(fdto, delta);
 	if (ret)
 		goto err;
 
+	/* Update fdto's phandles using symbols from fdt */
 	ret = overlay_fixup_phandles(fdt, fdto);
 	if (ret)
 		goto err;
 
+	/* Don't overwrite phandles in fdt */
+	ret = overlay_prevent_phandle_overwrite(fdt, fdto);
+	if (ret)
+		goto err;
+
 	ret = overlay_merge(fdt, fdto);
 	if (ret)
 		goto err;
diff --git a/tests/overlay_base_phandle.dts b/tests/overlay_base_phandle.dts
new file mode 100644
index 000000000000..20639a7f4026
--- /dev/null
+++ b/tests/overlay_base_phandle.dts
@@ -0,0 +1,21 @@
+/*
+ * Copyright (c) 2024 Uwe Kleine-König
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	node_a: a {
+		value = <17>;
+	};
+
+	node_b: b {
+		a = <&node_a>;
+	};
+
+	node_d: d {
+		b = <&node_b>;
+	};
+};
diff --git a/tests/overlay_overlay_phandle.dts b/tests/overlay_overlay_phandle.dts
new file mode 100644
index 000000000000..a0fa668faa5c
--- /dev/null
+++ b/tests/overlay_overlay_phandle.dts
@@ -0,0 +1,34 @@
+/*
+ * Copyright (c) 2024 Uwe Kleine-König
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	/*
+	 * /b already has a label "node_b" in the base dts, also b is already
+	 * referenced in the base, so both the base's b and this b have a
+	 * phandle value.
+	 */
+	node_b2: b {
+		value = <42>;
+		d = <&node_d>;
+	};
+
+	node_c: c {
+		value = <23>;
+		b = <&node_b2>;
+	};
+
+	d {
+		a = <&node_a>;
+		c = <&node_c>;
+	};
+};
+
+&node_a {
+	value = <32>;
+};
diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index 3225a12bbb06..f13cdb216d2b 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh
@@ -1040,6 +1040,34 @@ fdtoverlay_tests() {
     run_dtc_test -@ -I dts -O dtb -o $stacked_addlabeldtb $stacked_addlabel
 
     run_fdtoverlay_test baz "/foonode/barnode/baznode" "baz-property" "-ts" ${stacked_base_nolabeldtb} ${stacked_addlabel_targetdtb} ${stacked_addlabeldtb} ${stacked_bardtb} ${stacked_bazdtb}
+
+    # verify that phandles are not overwritten
+    run_dtc_test -@ -I dts -O dtb -o overlay_base_phandle.test.dtb "$SRCDIR/overlay_base_phandle.dts"
+    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_phandle.test.dtb "$SRCDIR/overlay_overlay_phandle.dts"
+    run_wrap_test $FDTOVERLAY -i overlay_base_phandle.test.dtb -o overlay_base_phandleO.test.dtb overlay_overlay_phandle.test.dtb
+
+    pa=$($DTGET overlay_base_phandleO.test.dtb /a phandle)
+    pb=$($DTGET overlay_base_phandleO.test.dtb /b phandle)
+    pc=$($DTGET overlay_base_phandleO.test.dtb /c phandle)
+    pd=$($DTGET overlay_base_phandleO.test.dtb /d phandle)
+    ba=$($DTGET overlay_base_phandleO.test.dtb /b a)
+    bd=$($DTGET overlay_base_phandleO.test.dtb /b d)
+    cb=$($DTGET overlay_base_phandleO.test.dtb /c b)
+    da=$($DTGET overlay_base_phandleO.test.dtb /d a)
+    db=$($DTGET overlay_base_phandleO.test.dtb /d b)
+    dc=$($DTGET overlay_base_phandleO.test.dtb /d c)
+
+    shorten_echo "check phandle to noda a wasn't overwritten:	"
+    run_wrap_test test "$ba-$da" = "$pa-$pa"
+
+    shorten_echo "check phandle to noda b wasn't overwritten:	"
+    run_wrap_test test "$cb-$db" = "$pb-$pb"
+
+    shorten_echo "check phandle to noda c wasn't overwritten:	"
+    run_wrap_test test "$dc" = "$pc"
+
+    shorten_echo "check phandle to noda d wasn't overwritten:	"
+    run_wrap_test test "$bd" = "$pd"
 }
 
 pylibfdt_tests () {

base-commit: 24f60011fd43683d8e3916435c4c726e9baac9c9
-- 
2.43.0


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