mail archive of the barebox mailing list
 help / color / mirror / Atom feed
* [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
@ 2021-06-28  6:40 Ahmad Fatoum
  2021-06-28  6:40 ` [PATCH 2/5] ARM: stm32mp: migrate to barebox,machine-id-path Ahmad Fatoum
                   ` (7 more replies)
  0 siblings, 8 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28  6:40 UTC (permalink / raw)
  To: barebox; +Cc: Bastian Krause, Uwe Kleine-König, Ahmad Fatoum

The Kconfig option already warns that the current behavior of
machine_id_set_hashable() overriding previous calls can lead to the
machine-id changing over updates. We don't yet have this problem in
practice, because the only two upstream users are for bsec and ocotp,
which are efuse drivers for different SoCs. On the other hand, users
may want to get the unique ID from an EEPROM and with deep probe
support, the initcall ordering will be independent of the actual probe
order.

Work around this issue by introducing a way for each board to explicitly
reference a nvmem cell that should be hashed to produce the machine-id.

If no such device tree property is supplied, the last call to
machine_id_set_hashable() will be used as before.

Cc: Bastian Krause <bst@pengutronix.de>
Cc: Uwe Kleine-König <ukl@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/Kconfig                 | 13 ++++++----
 common/machine_id.c            | 41 ++++++++++++++++++++++++++++---
 drivers/nvmem/core.c           | 44 ++++++++++++++++++++++++----------
 drivers/of/base.c              | 11 +++++++++
 include/linux/nvmem-consumer.h |  5 ++++
 include/of.h                   |  6 +++++
 6 files changed, 99 insertions(+), 21 deletions(-)

diff --git a/common/Kconfig b/common/Kconfig
index 54f1eb633a32..a4a109f04f08 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1066,13 +1066,16 @@ config MACHINE_ID
 	bool "compute unique machine-id"
 	depends on FLEXIBLE_BOOTARGS
 	depends on SHA1
+	select NVMEM
 	help
 	  Compute a persistent machine-specific id and store it to $global.machine_id.
-	  The id is a hash of device-specific information added via
-	  machine_id_set_hashable(). If multiple sources are available, the
-	  information provided by the last call prior to the late initcall
-	  set_machine_id() is used to generate the machine id from. Thus when
-	  updating barebox the machine id might change.
+	  The id is a hash of device-specific information added either via
+	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
+	  /chosen/barebox,machine-id device tree property.
+
+	  With machine_id_set_hashable(), the last call prior to the late initcall
+	  set_machine_id() willl be used to generate the machine id. This means
+	  updating barebox may change the machine id!
 
 	  global.bootm.provide_machine_id may be used to automatically set
 	  the linux.bootargs.machine_id global variable with a value of
diff --git a/common/machine_id.c b/common/machine_id.c
index 6480806cd287..fd2f0888a6cd 100644
--- a/common/machine_id.c
+++ b/common/machine_id.c
@@ -10,6 +10,9 @@
 #include <magicvar.h>
 #include <crypto/sha.h>
 #include <machine_id.h>
+#include <linux/err.h>
+#include <linux/nvmem-consumer.h>
+#include <of.h>
 
 #define MACHINE_ID_LENGTH 32
 
@@ -24,16 +27,49 @@ void machine_id_set_hashable(const void *hashable, size_t len)
 	__machine_id_hashable_length = len;
 }
 
+static const void *machine_id_get_hashable(size_t *len)
+{
+	struct device_node *np = of_get_machineidpath();
+	struct nvmem_cell *cell;
+	void *ret;
+
+	if (!np)
+		goto no_cell;
+
+	cell = of_nvmem_cell_get_from_node(np);
+	if (IS_ERR(cell)) {
+		pr_err("Invalid barebox,machine-id-path: %pe\n", cell);
+		goto no_cell;
+	}
+
+	ret = nvmem_cell_read(cell, len);
+	nvmem_cell_put(cell);
+	if (IS_ERR(ret)) {
+		pr_err("Couldn't read from barebox,machine-id-path: %pe\n", ret);
+		goto no_cell;
+	}
+
+	return ret;
+
+no_cell:
+	*len = __machine_id_hashable_length;
+	return __machine_id_hashable;
+}
+
 static int machine_id_set_globalvar(void)
 {
 	struct digest *digest = NULL;
 	unsigned char machine_id[SHA1_DIGEST_SIZE];
 	char hex_machine_id[MACHINE_ID_LENGTH];
 	char *env_machine_id;
+	const void *hashable;
+	size_t length;
 	int ret = 0;
 
+	hashable = machine_id_get_hashable(&length);
+
 	/* nothing to do if no hashable information provided */
-	if (!__machine_id_hashable)
+	if (!hashable)
 		goto out;
 
 	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
@@ -41,8 +77,7 @@ static int machine_id_set_globalvar(void)
 	if (ret)
 		goto out;
 
-	ret = digest_update(digest, __machine_id_hashable,
-			    __machine_id_hashable_length);
+	ret = digest_update(digest, hashable, length);
 	if (ret)
 		goto out;
 
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 4e558e165063..980304a8078b 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -361,29 +361,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
 /**
- * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
+ * of_nvmem_cell_get_from_node() - Get a nvmem cell from cell device node
  *
- * @dev node: Device tree node that uses the nvmem cell
- * @id: nvmem cell name from nvmem-cell-names property.
+ * @cell_np: Device tree node of the nvmem cell
  *
  * Return: Will be an ERR_PTR() on error or a valid pointer
  * to a struct nvmem_cell.  The nvmem_cell will be freed by the
  * nvmem_cell_put().
  */
-struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
-					    const char *name)
+struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
 {
-	struct device_node *cell_np, *nvmem_np;
+	struct device_node *nvmem_np;
 	struct nvmem_cell *cell;
 	struct nvmem_device *nvmem;
 	const __be32 *addr;
-	int rval, len, index;
-
-	index = of_property_match_string(np, "nvmem-cell-names", name);
-
-	cell_np = of_parse_phandle(np, "nvmem-cells", index);
-	if (!cell_np)
-		return ERR_PTR(-EINVAL);
+	int rval, len;
 
 	nvmem_np = of_get_parent(cell_np);
 	if (!nvmem_np)
@@ -445,6 +437,32 @@ err_mem:
 
 	return ERR_PTR(rval);
 }
+EXPORT_SYMBOL_GPL(of_nvmem_cell_get_from_node);
+
+/**
+ * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
+ *
+ * @dev node: Device tree node that uses the nvmem cell
+ * @id: nvmem cell name from nvmem-cell-names property.
+ *
+ * Return: Will be an ERR_PTR() on error or a valid pointer
+ * to a struct nvmem_cell.  The nvmem_cell will be freed by the
+ * nvmem_cell_put().
+ */
+struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
+					    const char *name)
+{
+	struct device_node *cell_np;
+	int index;
+
+	index = of_property_match_string(np, "nvmem-cell-names", name);
+
+	cell_np = of_parse_phandle(np, "nvmem-cells", index);
+	if (!cell_np)
+		return ERR_PTR(-EINVAL);
+
+	return of_nvmem_cell_get_from_node(cell_np);
+}
 EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
 #endif
 
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 4104d7589305..a5b74707fc4f 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2525,6 +2525,17 @@ int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate)
 	return true;
 }
 
+struct device_node *of_get_machineidpath(void)
+{
+	const char *name;
+
+	name = of_get_property(of_chosen, "barebox,machine-id-path", NULL);
+	if (!name)
+		return NULL;
+
+	return of_find_node_by_path_or_alias(NULL, name);
+}
+
 /**
  * of_add_initrd - add initrd properties to the devicetree
  * @root - the root node of the tree
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index b979f23372a6..639a7ebbabae 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -122,11 +122,16 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
 #endif /* CONFIG_NVMEM */
 
 #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
+struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np);
 struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 				     const char *name);
 struct nvmem_device *of_nvmem_device_get(struct device_node *np,
 					 const char *name);
 #else
+static inline struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
+{
+	return ERR_PTR(-ENOSYS);
+}
 static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
 				     const char *name)
 {
diff --git a/include/of.h b/include/of.h
index d82790c0523e..677f48d0aba1 100644
--- a/include/of.h
+++ b/include/of.h
@@ -290,6 +290,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev);
 int of_partitions_register_fixup(struct cdev *cdev);
 struct device_node *of_get_stdoutpath(unsigned int *);
 int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate);
+struct device_node *of_get_machineidpath(void);
 const char *of_get_model(void);
 void *of_flatten_dtb(struct device_node *node);
 int of_add_memory(struct device_node *node, bool dump);
@@ -336,6 +337,11 @@ static inline int of_device_is_stdout_path(struct device_d *dev, unsigned int *b
 	return 0;
 }
 
+static inline struct device_node *of_get_machineidpath(void)
+{
+	return NULL;
+}
+
 static inline const char *of_get_model(void)
 {
 	return NULL;
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

* [PATCH 2/5] ARM: stm32mp: migrate to barebox,machine-id-path
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
@ 2021-06-28  6:40 ` Ahmad Fatoum
  2021-06-28  6:40 ` [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable Ahmad Fatoum
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28  6:40 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

All STM32MP1 boards use a device tree that includes stm32mp151.dtsi, so
we can just make use of the new property there and drop machine_id_set_hashable
without affecting behavior.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/dts/stm32mp151.dtsi |  8 ++++++++
 drivers/nvmem/bsec.c         | 17 -----------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/arch/arm/dts/stm32mp151.dtsi b/arch/arm/dts/stm32mp151.dtsi
index f1fd888fa1c6..eb3c6222e785 100644
--- a/arch/arm/dts/stm32mp151.dtsi
+++ b/arch/arm/dts/stm32mp151.dtsi
@@ -31,6 +31,10 @@
 		tamp.reboot_mode = &reboot_mode_tamp;
 	};
 
+	chosen {
+		barebox,machine-id-path = &otp_serial;
+	};
+
 };
 
 &{/clocks} {
@@ -61,6 +65,10 @@
 
 &bsec {
 	barebox,provide-mac-address = <&ethernet0 0x39>;
+
+	otp_serial: serial-number@34 {
+		reg = <0x34 0xc>;
+	};
 };
 
 &vrefbuf {
diff --git a/drivers/nvmem/bsec.c b/drivers/nvmem/bsec.c
index d9b38c8414fb..097d34069afd 100644
--- a/drivers/nvmem/bsec.c
+++ b/drivers/nvmem/bsec.c
@@ -15,7 +15,6 @@
 #include <of.h>
 #include <regmap.h>
 #include <mach/bsec.h>
-#include <machine_id.h>
 #include <linux/nvmem-provider.h>
 
 #define BSEC_OTP_SERIAL	13
@@ -69,19 +68,6 @@ static struct regmap_bus stm32_bsec_regmap_bus = {
 	.reg_read = stm32_bsec_read_shadow,
 };
 
-static void stm32_bsec_set_unique_machine_id(struct regmap *map)
-{
-	u32 unique_id[3];
-	int ret;
-
-	ret = regmap_bulk_read(map, BSEC_OTP_SERIAL * 4,
-			       unique_id, sizeof(unique_id));
-	if (ret)
-		return;
-
-	machine_id_set_hashable(unique_id, sizeof(unique_id));
-}
-
 static int stm32_bsec_read_mac(struct regmap *map, int offset, u8 *mac)
 {
 	u8 res[8];
@@ -156,9 +142,6 @@ static int stm32_bsec_probe(struct device_d *dev)
 	if (IS_ERR(nvmem))
 		return PTR_ERR(nvmem);
 
-	if (IS_ENABLED(CONFIG_MACHINE_ID))
-		stm32_bsec_set_unique_machine_id(map);
-
 	stm32_bsec_init_dt(dev, map);
 
 	return 0;
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
  2021-06-28  6:40 ` [PATCH 2/5] ARM: stm32mp: migrate to barebox,machine-id-path Ahmad Fatoum
@ 2021-06-28  6:40 ` Ahmad Fatoum
  2021-06-28  9:50   ` Bastian Krause
  2021-06-28  6:40 ` [PATCH 4/5] sandbox: dts: populate $global.machine_id Ahmad Fatoum
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28  6:40 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

The Kconfig symbol already warns users that barebox updates
that add new instances of machine_id_set_hashable may cause the machine
ID to change making the future less useful. Recent changes allow the
board DT to identify a specific nvmem cell to use for supplying a machine
ID, making the old way of drivers providing machine IDs and possibly
overriding each other no longer necessary or recommended.

Make this explicit by allowing the old code to be disabled. As we only
have a single user now and won't accept any new ones, we can remove the
warning about it possibly changing after update.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 common/Kconfig        | 18 +++++++++++-------
 common/machine_id.c   | 11 ++++++++---
 drivers/nvmem/Kconfig |  1 +
 drivers/nvmem/ocotp.c |  7 ++++++-
 include/machine_id.h  | 16 ----------------
 5 files changed, 26 insertions(+), 27 deletions(-)
 delete mode 100644 include/machine_id.h

diff --git a/common/Kconfig b/common/Kconfig
index a4a109f04f08..a2aa0b2568de 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1069,13 +1069,11 @@ config MACHINE_ID
 	select NVMEM
 	help
 	  Compute a persistent machine-specific id and store it to $global.machine_id.
-	  The id is a hash of device-specific information added either via
-	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
-	  /chosen/barebox,machine-id device tree property.
-
-	  With machine_id_set_hashable(), the last call prior to the late initcall
-	  set_machine_id() willl be used to generate the machine id. This means
-	  updating barebox may change the machine id!
+	  The id is a hash of device-specific information. This information
+	  comes from the nvmem cell device tree node path described by the
+	  /chosen/barebox,machine-id-path property. As a special case, the i.MX6 OCOTP
+	  driver supplies the SoC UID as hashable for when /chosen/barebox,machine-id-path
+	  is not specified.
 
 	  global.bootm.provide_machine_id may be used to automatically set
 	  the linux.bootargs.machine_id global variable with a value of
@@ -1084,6 +1082,12 @@ config MACHINE_ID
 	  Note: if no hashable information is available no machine id will be passed
 	  to the kernel.
 
+config MACHINE_ID_LEGACY
+	bool
+	help
+	  Selected by i.MX OCOTP driver, so it can set the SoC UID as hashable.
+	  New platforms should use /chosen/barebox,machine-id-path instead.
+
 config SYSTEMD_OF_WATCHDOG
 	bool "inform devicetree-enabled kernel of used watchdog"
 	depends on WATCHDOG && OFTREE && FLEXIBLE_BOOTARGS
diff --git a/common/machine_id.c b/common/machine_id.c
index fd2f0888a6cd..8303c0b7aa87 100644
--- a/common/machine_id.c
+++ b/common/machine_id.c
@@ -9,23 +9,24 @@
 #include <globalvar.h>
 #include <magicvar.h>
 #include <crypto/sha.h>
-#include <machine_id.h>
 #include <linux/err.h>
 #include <linux/nvmem-consumer.h>
 #include <of.h>
 
 #define MACHINE_ID_LENGTH 32
 
+#ifdef CONFIG_MACHINE_ID_LEGACY
 static void *__machine_id_hashable;
 static size_t __machine_id_hashable_length;
 
-
+/* Shouldn't be called from new code */
+void machine_id_set_hashable(const void *hashable, size_t len);
 void machine_id_set_hashable(const void *hashable, size_t len)
 {
-
 	__machine_id_hashable = xmemdup(hashable, len);
 	__machine_id_hashable_length = len;
 }
+#endif
 
 static const void *machine_id_get_hashable(size_t *len)
 {
@@ -52,8 +53,12 @@ static const void *machine_id_get_hashable(size_t *len)
 	return ret;
 
 no_cell:
+#ifdef CONFIG_MACHINE_ID_LEGACY
 	*len = __machine_id_hashable_length;
 	return __machine_id_hashable;
+#else
+	return NULL;
+#endif
 }
 
 static int machine_id_set_globalvar(void)
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 3781f7a839fc..08a5765573a8 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -25,6 +25,7 @@ config NVMEM_SNVS_LPGPR
 config IMX_OCOTP
 	tristate "i.MX6 On Chip OTP controller"
 	depends on ARCH_IMX6 || ARCH_VF610 || ARCH_IMX8M || ARCH_IMX7
+	select MACHINE_ID_LEGACY
 	depends on OFDEVICE
 	help
 	  This adds support for the i.MX6 On-Chip OTP controller. Currently the
diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
index b2fad3c68770..0b10e52a86ba 100644
--- a/drivers/nvmem/ocotp.c
+++ b/drivers/nvmem/ocotp.c
@@ -29,7 +29,6 @@
 #include <regmap.h>
 #include <linux/clk.h>
 #include <mach/ocotp.h>
-#include <machine_id.h>
 #include <mach/ocotp-fusemap.h>
 #include <linux/nvmem-provider.h>
 
@@ -689,6 +688,7 @@ static int imx_ocotp_read(void *ctx, unsigned offset, void *val, size_t bytes)
 
 static void imx_ocotp_set_unique_machine_id(void)
 {
+	extern void machine_id_set_hashable(const void *hashable, size_t len);
 	uint32_t unique_id_parts[UNIQUE_ID_NUM];
 	int i;
 
@@ -785,6 +785,11 @@ static int imx_ocotp_probe(struct device_d *dev)
 				  ethaddr->value, ethaddr);
 	}
 
+	/* Special case: new machine id providers should provide a nvmem cell
+	 * and reference its path via /chosen/barebox,machine-id-path.
+	 * For i.MX, we support the legacy way of the driver supplying the
+	 * hash instead
+	 */
 	if (IS_ENABLED(CONFIG_MACHINE_ID))
 		imx_ocotp_set_unique_machine_id();
 
diff --git a/include/machine_id.h b/include/machine_id.h
deleted file mode 100644
index 31d5e0bb2851..000000000000
--- a/include/machine_id.h
+++ /dev/null
@@ -1,16 +0,0 @@
-#ifndef __MACHINE_ID_H__
-#define __MACHINE_ID_H__
-
-#if IS_ENABLED(CONFIG_MACHINE_ID)
-
-void machine_id_set_hashable(const void *hashable, size_t len);
-
-#else
-
-static inline void machine_id_set_hashable(const void *hashable, size_t len)
-{
-}
-
-#endif /* CONFIG_MACHINE_ID */
-
-#endif  /* __MACHINE_ID_H__ */
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 4/5] sandbox: dts: populate $global.machine_id
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
  2021-06-28  6:40 ` [PATCH 2/5] ARM: stm32mp: migrate to barebox,machine-id-path Ahmad Fatoum
  2021-06-28  6:40 ` [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable Ahmad Fatoum
@ 2021-06-28  6:40 ` Ahmad Fatoum
  2021-06-28  6:40 ` [PATCH 5/5] ARM: dts: stm32mp: retire barebox, provide-mac-address in favor of NVMEM Ahmad Fatoum
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28  6:40 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

/etc/machine-id is 0444 by default, so it makes sense to use it for
sandbox machine ID. Do so.

Note that the machine ID will be constant, but not equal as barebox
will hash the ASCII representation.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/sandbox/dts/sandbox.dts | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/arch/sandbox/dts/sandbox.dts b/arch/sandbox/dts/sandbox.dts
index 5b2cab219e2a..cd0022f777ed 100644
--- a/arch/sandbox/dts/sandbox.dts
+++ b/arch/sandbox/dts/sandbox.dts
@@ -13,6 +13,7 @@
 	};
 
 	chosen {
+		barebox,machine-id-path = &machine_id;
 		environment {
 			compatible = "barebox,environment";
 			device-path = &part_env;
@@ -101,6 +102,33 @@
 		};
 	};
 
+	machine-id {
+		compatible = "barebox,hostfile";
+		barebox,filename = "/etc/machine-id";
+		barebox,read-only;
+		reg = <0 0 0 0>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			partition@0 {
+				compatible = "nvmem-cells";
+				reg = <0x0 0x20>;
+				label = "nvmem";
+				#address-cells = <1>;
+				#size-cells = <1>;
+
+				machine_id: machine-id@0 {
+					reg = <0x0 0x20>;
+				};
+			};
+		};
+	};
+
 	power {
 		compatible = "barebox,sandbox-power";
 		nvmem-cell-names = "reset-source";
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* [PATCH 5/5] ARM: dts: stm32mp: retire barebox, provide-mac-address in favor of NVMEM
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
                   ` (2 preceding siblings ...)
  2021-06-28  6:40 ` [PATCH 4/5] sandbox: dts: populate $global.machine_id Ahmad Fatoum
@ 2021-06-28  6:40 ` Ahmad Fatoum
  2021-06-28  9:28 ` [PATCH 1/5] common: machine_id: support /chosen/barebox,machine-id-path override Ahmad Fatoum
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28  6:40 UTC (permalink / raw)
  To: barebox; +Cc: Ahmad Fatoum

We now have generic support for populating MAC address out of NVMEM, so
use it instead of barebox,provide-mac-address. There should be no
functional change.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 arch/arm/dts/stm32mp151.dtsi | 11 ++++++--
 drivers/nvmem/bsec.c         | 51 +-----------------------------------
 2 files changed, 10 insertions(+), 52 deletions(-)

diff --git a/arch/arm/dts/stm32mp151.dtsi b/arch/arm/dts/stm32mp151.dtsi
index eb3c6222e785..65df4ca44115 100644
--- a/arch/arm/dts/stm32mp151.dtsi
+++ b/arch/arm/dts/stm32mp151.dtsi
@@ -64,11 +64,18 @@
 };
 
 &bsec {
-	barebox,provide-mac-address = <&ethernet0 0x39>;
-
 	otp_serial: serial-number@34 {
 		reg = <0x34 0xc>;
 	};
+
+	macaddr: mac-address@e4 {
+		reg = <0xe4 0x6>;
+	};
+};
+
+&ethernet0 {
+	nvmem-cell-names = "mac-address";
+	nvmem-cells = <&macaddr>;
 };
 
 &vrefbuf {
diff --git a/drivers/nvmem/bsec.c b/drivers/nvmem/bsec.c
index 097d34069afd..0ed4d089d46b 100644
--- a/drivers/nvmem/bsec.c
+++ b/drivers/nvmem/bsec.c
@@ -68,51 +68,6 @@ static struct regmap_bus stm32_bsec_regmap_bus = {
 	.reg_read = stm32_bsec_read_shadow,
 };
 
-static int stm32_bsec_read_mac(struct regmap *map, int offset, u8 *mac)
-{
-	u8 res[8];
-	int ret;
-
-	ret = regmap_bulk_read(map, offset * 4, res, 8);
-	if (ret)
-		return ret;
-
-	memcpy(mac, res, ETH_ALEN);
-	return 0;
-}
-
-static void stm32_bsec_init_dt(struct device_d *dev, struct regmap *map)
-{
-	struct device_node *node = dev->device_node;
-	struct device_node *rnode;
-	u32 phandle, offset;
-	char mac[ETH_ALEN];
-	const __be32 *prop;
-
-	int len;
-	int ret;
-
-	prop = of_get_property(node, "barebox,provide-mac-address", &len);
-	if (!prop)
-		return;
-
-	if (len != 2 * sizeof(__be32))
-		return;
-
-	phandle = be32_to_cpup(prop++);
-
-	rnode = of_find_node_by_phandle(phandle);
-	offset = be32_to_cpup(prop++);
-
-	ret = stm32_bsec_read_mac(map, offset, mac);
-	if (ret) {
-		dev_warn(dev, "error setting MAC address: %s\n", strerror(-ret));
-		return;
-	}
-
-	of_eth_register_ethaddr(rnode, mac);
-}
-
 static int stm32_bsec_probe(struct device_d *dev)
 {
 	struct regmap *map;
@@ -139,12 +94,8 @@ static int stm32_bsec_probe(struct device_d *dev)
 		return PTR_ERR(map);
 
 	nvmem = nvmem_regmap_register(map, "stm32-bsec");
-	if (IS_ERR(nvmem))
-		return PTR_ERR(nvmem);
-
-	stm32_bsec_init_dt(dev, map);
 
-	return 0;
+	return PTR_ERR_OR_ZERO(nvmem);
 }
 
 static struct stm32_bsec_data stm32mp15_bsec_data = {
-- 
2.30.2


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox,machine-id-path override
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
                   ` (3 preceding siblings ...)
  2021-06-28  6:40 ` [PATCH 5/5] ARM: dts: stm32mp: retire barebox, provide-mac-address in favor of NVMEM Ahmad Fatoum
@ 2021-06-28  9:28 ` Ahmad Fatoum
  2021-06-28  9:35 ` [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Bastian Krause
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28  9:28 UTC (permalink / raw)
  To: barebox; +Cc: Bastian Krause, Uwe Kleine-König

On 28.06.21 08:40, Ahmad Fatoum wrote:
> diff --git a/include/of.h b/include/of.h
> index d82790c0523e..677f48d0aba1 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -290,6 +290,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev);
>  int of_partitions_register_fixup(struct cdev *cdev);
>  struct device_node *of_get_stdoutpath(unsigned int *);
>  int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate);

These were added here:
https://lore.pengutronix.de/barebox/20210628051934.9604-1-a.fatoum@pengutronix.de/T/#t

Those two patches are required for this series to apply cleanly.

> +struct device_node *of_get_machineidpath(void);
>  const char *of_get_model(void);
>  void *of_flatten_dtb(struct device_node *node);
>  int of_add_memory(struct device_node *node, bool dump);
> @@ -336,6 +337,11 @@ static inline int of_device_is_stdout_path(struct device_d *dev, unsigned int *b
>  	return 0;
>  }
>  
> +static inline struct device_node *of_get_machineidpath(void)
> +{
> +	return NULL;
> +}
> +
>  static inline const char *of_get_model(void)
>  {
>  	return NULL;
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
                   ` (4 preceding siblings ...)
  2021-06-28  9:28 ` [PATCH 1/5] common: machine_id: support /chosen/barebox,machine-id-path override Ahmad Fatoum
@ 2021-06-28  9:35 ` Bastian Krause
  2021-06-28 10:11   ` Ahmad Fatoum
  2021-06-28 20:20 ` Sascha Hauer
       [not found] ` <CAMHeXxOT__KBUKG6GkNAEkqz4tMBBzuZ7OgnKa0_OX5hz-JEig@mail.gmail.com>
  7 siblings, 1 reply; 14+ messages in thread
From: Bastian Krause @ 2021-06-28  9:35 UTC (permalink / raw)
  To: Ahmad Fatoum, barebox; +Cc: Uwe Kleine-König

On 6/28/21 8:40 AM, Ahmad Fatoum wrote:
> The Kconfig option already warns that the current behavior of
> machine_id_set_hashable() overriding previous calls can lead to the
> machine-id changing over updates. We don't yet have this problem in
> practice, because the only two upstream users are for bsec and ocotp,
> which are efuse drivers for different SoCs. On the other hand, users
> may want to get the unique ID from an EEPROM and with deep probe
> support, the initcall ordering will be independent of the actual probe
> order.
> 
> Work around this issue by introducing a way for each board to explicitly
> reference a nvmem cell that should be hashed to produce the machine-id.
> 
> If no such device tree property is supplied, the last call to
> machine_id_set_hashable() will be used as before.
> 
> Cc: Bastian Krause <bst@pengutronix.de>
> Cc: Uwe Kleine-König <ukl@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/Kconfig                 | 13 ++++++----
>  common/machine_id.c            | 41 ++++++++++++++++++++++++++++---
>  drivers/nvmem/core.c           | 44 ++++++++++++++++++++++++----------
>  drivers/of/base.c              | 11 +++++++++
>  include/linux/nvmem-consumer.h |  5 ++++
>  include/of.h                   |  6 +++++
>  6 files changed, 99 insertions(+), 21 deletions(-)
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index 54f1eb633a32..a4a109f04f08 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1066,13 +1066,16 @@ config MACHINE_ID
>  	bool "compute unique machine-id"
>  	depends on FLEXIBLE_BOOTARGS
>  	depends on SHA1
> +	select NVMEM
>  	help
>  	  Compute a persistent machine-specific id and store it to $global.machine_id.
> -	  The id is a hash of device-specific information added via
> -	  machine_id_set_hashable(). If multiple sources are available, the
> -	  information provided by the last call prior to the late initcall
> -	  set_machine_id() is used to generate the machine id from. Thus when
> -	  updating barebox the machine id might change.
> +	  The id is a hash of device-specific information added either via
> +	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
> +	  /chosen/barebox,machine-id device tree property.
> +
> +	  With machine_id_set_hashable(), the last call prior to the late initcall
> +	  set_machine_id() willl be used to generate the machine id. This means
> +	  updating barebox may change the machine id!
>  
>  	  global.bootm.provide_machine_id may be used to automatically set
>  	  the linux.bootargs.machine_id global variable with a value of
> diff --git a/common/machine_id.c b/common/machine_id.c
> index 6480806cd287..fd2f0888a6cd 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -10,6 +10,9 @@
>  #include <magicvar.h>
>  #include <crypto/sha.h>
>  #include <machine_id.h>
> +#include <linux/err.h>
> +#include <linux/nvmem-consumer.h>
> +#include <of.h>
>  
>  #define MACHINE_ID_LENGTH 32
>  
> @@ -24,16 +27,49 @@ void machine_id_set_hashable(const void *hashable, size_t len)
>  	__machine_id_hashable_length = len;
>  }
>  
> +static const void *machine_id_get_hashable(size_t *len)
> +{
> +	struct device_node *np = of_get_machineidpath();
> +	struct nvmem_cell *cell;
> +	void *ret;
> +
> +	if (!np)
> +		goto no_cell;
> +
> +	cell = of_nvmem_cell_get_from_node(np);
> +	if (IS_ERR(cell)) {
> +		pr_err("Invalid barebox,machine-id-path: %pe\n", cell);
> +		goto no_cell;
> +	}
> +
> +	ret = nvmem_cell_read(cell, len);
> +	nvmem_cell_put(cell);
> +	if (IS_ERR(ret)) {
> +		pr_err("Couldn't read from barebox,machine-id-path: %pe\n", ret);
> +		goto no_cell;
> +	}
> +
> +	return ret;
> +
> +no_cell:
> +	*len = __machine_id_hashable_length;
> +	return __machine_id_hashable;
> +}
> +
>  static int machine_id_set_globalvar(void)
>  {
>  	struct digest *digest = NULL;
>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>  	char hex_machine_id[MACHINE_ID_LENGTH];
>  	char *env_machine_id;
> +	const void *hashable;
> +	size_t length;
>  	int ret = 0;
>  
> +	hashable = machine_id_get_hashable(&length);
> +
>  	/* nothing to do if no hashable information provided */
> -	if (!__machine_id_hashable)
> +	if (!hashable)
>  		goto out;
>  
>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
> @@ -41,8 +77,7 @@ static int machine_id_set_globalvar(void)
>  	if (ret)
>  		goto out;
>  
> -	ret = digest_update(digest, __machine_id_hashable,
> -			    __machine_id_hashable_length);
> +	ret = digest_update(digest, hashable, length);
>  	if (ret)
>  		goto out;
>  
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 4e558e165063..980304a8078b 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -361,29 +361,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
>  
>  #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
>  /**
> - * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
> + * of_nvmem_cell_get_from_node() - Get a nvmem cell from cell device node
>   *
> - * @dev node: Device tree node that uses the nvmem cell
> - * @id: nvmem cell name from nvmem-cell-names property.
> + * @cell_np: Device tree node of the nvmem cell
>   *
>   * Return: Will be an ERR_PTR() on error or a valid pointer
>   * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>   * nvmem_cell_put().
>   */
> -struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> -					    const char *name)
> +struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
>  {
> -	struct device_node *cell_np, *nvmem_np;
> +	struct device_node *nvmem_np;
>  	struct nvmem_cell *cell;
>  	struct nvmem_device *nvmem;
>  	const __be32 *addr;
> -	int rval, len, index;
> -
> -	index = of_property_match_string(np, "nvmem-cell-names", name);
> -
> -	cell_np = of_parse_phandle(np, "nvmem-cells", index);
> -	if (!cell_np)
> -		return ERR_PTR(-EINVAL);
> +	int rval, len;
>  
>  	nvmem_np = of_get_parent(cell_np);
>  	if (!nvmem_np)
> @@ -445,6 +437,32 @@ err_mem:
>  
>  	return ERR_PTR(rval);
>  }
> +EXPORT_SYMBOL_GPL(of_nvmem_cell_get_from_node);
> +
> +/**
> + * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
> + *
> + * @dev node: Device tree node that uses the nvmem cell
> + * @id: nvmem cell name from nvmem-cell-names property.
> + *
> + * Return: Will be an ERR_PTR() on error or a valid pointer
> + * to a struct nvmem_cell.  The nvmem_cell will be freed by the
> + * nvmem_cell_put().
> + */
> +struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
> +					    const char *name)
> +{
> +	struct device_node *cell_np;
> +	int index;
> +
> +	index = of_property_match_string(np, "nvmem-cell-names", name);
> +
> +	cell_np = of_parse_phandle(np, "nvmem-cells", index);
> +	if (!cell_np)
> +		return ERR_PTR(-EINVAL);
> +
> +	return of_nvmem_cell_get_from_node(cell_np);
> +}
>  EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>  #endif

Shouldn't these changes be part of a separate patch?

Regards,
Bastian

>  
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 4104d7589305..a5b74707fc4f 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2525,6 +2525,17 @@ int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate)
>  	return true;
>  }
>  
> +struct device_node *of_get_machineidpath(void)
> +{
> +	const char *name;
> +
> +	name = of_get_property(of_chosen, "barebox,machine-id-path", NULL);
> +	if (!name)
> +		return NULL;
> +
> +	return of_find_node_by_path_or_alias(NULL, name);
> +}
> +
>  /**
>   * of_add_initrd - add initrd properties to the devicetree
>   * @root - the root node of the tree
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index b979f23372a6..639a7ebbabae 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -122,11 +122,16 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
>  #endif /* CONFIG_NVMEM */
>  
>  #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
> +struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np);
>  struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>  				     const char *name);
>  struct nvmem_device *of_nvmem_device_get(struct device_node *np,
>  					 const char *name);
>  #else
> +static inline struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
> +{
> +	return ERR_PTR(-ENOSYS);
> +}
>  static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>  				     const char *name)
>  {
> diff --git a/include/of.h b/include/of.h
> index d82790c0523e..677f48d0aba1 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -290,6 +290,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev);
>  int of_partitions_register_fixup(struct cdev *cdev);
>  struct device_node *of_get_stdoutpath(unsigned int *);
>  int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate);
> +struct device_node *of_get_machineidpath(void);
>  const char *of_get_model(void);
>  void *of_flatten_dtb(struct device_node *node);
>  int of_add_memory(struct device_node *node, bool dump);
> @@ -336,6 +337,11 @@ static inline int of_device_is_stdout_path(struct device_d *dev, unsigned int *b
>  	return 0;
>  }
>  
> +static inline struct device_node *of_get_machineidpath(void)
> +{
> +	return NULL;
> +}
> +
>  static inline const char *of_get_model(void)
>  {
>  	return NULL;
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

* Re: [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable
  2021-06-28  6:40 ` [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable Ahmad Fatoum
@ 2021-06-28  9:50   ` Bastian Krause
  2021-06-28 10:12     ` Ahmad Fatoum
  0 siblings, 1 reply; 14+ messages in thread
From: Bastian Krause @ 2021-06-28  9:50 UTC (permalink / raw)
  To: Ahmad Fatoum, barebox

On 6/28/21 8:40 AM, Ahmad Fatoum wrote:
> The Kconfig symbol already warns users that barebox updates
> that add new instances of machine_id_set_hashable may cause the machine
> ID to change making the future less useful. Recent changes allow the
> board DT to identify a specific nvmem cell to use for supplying a machine
> ID, making the old way of drivers providing machine IDs and possibly
> overriding each other no longer necessary or recommended.

I cannot parse the first sentence of the commit message.

Regards,
Bastian

> 
> Make this explicit by allowing the old code to be disabled. As we only
> have a single user now and won't accept any new ones, we can remove the
> warning about it possibly changing after update.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/Kconfig        | 18 +++++++++++-------
>  common/machine_id.c   | 11 ++++++++---
>  drivers/nvmem/Kconfig |  1 +
>  drivers/nvmem/ocotp.c |  7 ++++++-
>  include/machine_id.h  | 16 ----------------
>  5 files changed, 26 insertions(+), 27 deletions(-)
>  delete mode 100644 include/machine_id.h
> 
> diff --git a/common/Kconfig b/common/Kconfig
> index a4a109f04f08..a2aa0b2568de 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1069,13 +1069,11 @@ config MACHINE_ID
>  	select NVMEM
>  	help
>  	  Compute a persistent machine-specific id and store it to $global.machine_id.
> -	  The id is a hash of device-specific information added either via
> -	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
> -	  /chosen/barebox,machine-id device tree property.
> -
> -	  With machine_id_set_hashable(), the last call prior to the late initcall
> -	  set_machine_id() willl be used to generate the machine id. This means
> -	  updating barebox may change the machine id!
> +	  The id is a hash of device-specific information. This information
> +	  comes from the nvmem cell device tree node path described by the
> +	  /chosen/barebox,machine-id-path property. As a special case, the i.MX6 OCOTP
> +	  driver supplies the SoC UID as hashable for when /chosen/barebox,machine-id-path
> +	  is not specified.
>  
>  	  global.bootm.provide_machine_id may be used to automatically set
>  	  the linux.bootargs.machine_id global variable with a value of
> @@ -1084,6 +1082,12 @@ config MACHINE_ID
>  	  Note: if no hashable information is available no machine id will be passed
>  	  to the kernel.
>  
> +config MACHINE_ID_LEGACY
> +	bool
> +	help
> +	  Selected by i.MX OCOTP driver, so it can set the SoC UID as hashable.
> +	  New platforms should use /chosen/barebox,machine-id-path instead.
> +
>  config SYSTEMD_OF_WATCHDOG
>  	bool "inform devicetree-enabled kernel of used watchdog"
>  	depends on WATCHDOG && OFTREE && FLEXIBLE_BOOTARGS
> diff --git a/common/machine_id.c b/common/machine_id.c
> index fd2f0888a6cd..8303c0b7aa87 100644
> --- a/common/machine_id.c
> +++ b/common/machine_id.c
> @@ -9,23 +9,24 @@
>  #include <globalvar.h>
>  #include <magicvar.h>
>  #include <crypto/sha.h>
> -#include <machine_id.h>
>  #include <linux/err.h>
>  #include <linux/nvmem-consumer.h>
>  #include <of.h>
>  
>  #define MACHINE_ID_LENGTH 32
>  
> +#ifdef CONFIG_MACHINE_ID_LEGACY
>  static void *__machine_id_hashable;
>  static size_t __machine_id_hashable_length;
>  
> -
> +/* Shouldn't be called from new code */
> +void machine_id_set_hashable(const void *hashable, size_t len);
>  void machine_id_set_hashable(const void *hashable, size_t len)
>  {
> -
>  	__machine_id_hashable = xmemdup(hashable, len);
>  	__machine_id_hashable_length = len;
>  }
> +#endif
>  
>  static const void *machine_id_get_hashable(size_t *len)
>  {
> @@ -52,8 +53,12 @@ static const void *machine_id_get_hashable(size_t *len)
>  	return ret;
>  
>  no_cell:
> +#ifdef CONFIG_MACHINE_ID_LEGACY
>  	*len = __machine_id_hashable_length;
>  	return __machine_id_hashable;
> +#else
> +	return NULL;
> +#endif
>  }
>  
>  static int machine_id_set_globalvar(void)
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 3781f7a839fc..08a5765573a8 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -25,6 +25,7 @@ config NVMEM_SNVS_LPGPR
>  config IMX_OCOTP
>  	tristate "i.MX6 On Chip OTP controller"
>  	depends on ARCH_IMX6 || ARCH_VF610 || ARCH_IMX8M || ARCH_IMX7
> +	select MACHINE_ID_LEGACY
>  	depends on OFDEVICE
>  	help
>  	  This adds support for the i.MX6 On-Chip OTP controller. Currently the
> diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
> index b2fad3c68770..0b10e52a86ba 100644
> --- a/drivers/nvmem/ocotp.c
> +++ b/drivers/nvmem/ocotp.c
> @@ -29,7 +29,6 @@
>  #include <regmap.h>
>  #include <linux/clk.h>
>  #include <mach/ocotp.h>
> -#include <machine_id.h>
>  #include <mach/ocotp-fusemap.h>
>  #include <linux/nvmem-provider.h>
>  
> @@ -689,6 +688,7 @@ static int imx_ocotp_read(void *ctx, unsigned offset, void *val, size_t bytes)
>  
>  static void imx_ocotp_set_unique_machine_id(void)
>  {
> +	extern void machine_id_set_hashable(const void *hashable, size_t len);
>  	uint32_t unique_id_parts[UNIQUE_ID_NUM];
>  	int i;
>  
> @@ -785,6 +785,11 @@ static int imx_ocotp_probe(struct device_d *dev)
>  				  ethaddr->value, ethaddr);
>  	}
>  
> +	/* Special case: new machine id providers should provide a nvmem cell
> +	 * and reference its path via /chosen/barebox,machine-id-path.
> +	 * For i.MX, we support the legacy way of the driver supplying the
> +	 * hash instead
> +	 */
>  	if (IS_ENABLED(CONFIG_MACHINE_ID))
>  		imx_ocotp_set_unique_machine_id();
>  
> diff --git a/include/machine_id.h b/include/machine_id.h
> deleted file mode 100644
> index 31d5e0bb2851..000000000000
> --- a/include/machine_id.h
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -#ifndef __MACHINE_ID_H__
> -#define __MACHINE_ID_H__
> -
> -#if IS_ENABLED(CONFIG_MACHINE_ID)
> -
> -void machine_id_set_hashable(const void *hashable, size_t len);
> -
> -#else
> -
> -static inline void machine_id_set_hashable(const void *hashable, size_t len)
> -{
> -}
> -
> -#endif /* CONFIG_MACHINE_ID */
> -
> -#endif  /* __MACHINE_ID_H__ */
> 


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
  2021-06-28  9:35 ` [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Bastian Krause
@ 2021-06-28 10:11   ` Ahmad Fatoum
  0 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28 10:11 UTC (permalink / raw)
  To: Bastian Krause, barebox; +Cc: Uwe Kleine-König

Hello Basti,

On 28.06.21 11:35, Bastian Krause wrote:
> On 6/28/21 8:40 AM, Ahmad Fatoum wrote:
>> The Kconfig option already warns that the current behavior of
>> machine_id_set_hashable() overriding previous calls can lead to the
>> machine-id changing over updates. We don't yet have this problem in
>> practice, because the only two upstream users are for bsec and ocotp,
>> which are efuse drivers for different SoCs. On the other hand, users
>> may want to get the unique ID from an EEPROM and with deep probe
>> support, the initcall ordering will be independent of the actual probe
>> order.
>>
>> Work around this issue by introducing a way for each board to explicitly
>> reference a nvmem cell that should be hashed to produce the machine-id.
>>
>> If no such device tree property is supplied, the last call to
>> machine_id_set_hashable() will be used as before.
>>
>> Cc: Bastian Krause <bst@pengutronix.de>
>> Cc: Uwe Kleine-König <ukl@pengutronix.de>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/Kconfig                 | 13 ++++++----
>>  common/machine_id.c            | 41 ++++++++++++++++++++++++++++---
>>  drivers/nvmem/core.c           | 44 ++++++++++++++++++++++++----------
>>  drivers/of/base.c              | 11 +++++++++
>>  include/linux/nvmem-consumer.h |  5 ++++
>>  include/of.h                   |  6 +++++
>>  6 files changed, 99 insertions(+), 21 deletions(-)
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index 54f1eb633a32..a4a109f04f08 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -1066,13 +1066,16 @@ config MACHINE_ID
>>  	bool "compute unique machine-id"
>>  	depends on FLEXIBLE_BOOTARGS
>>  	depends on SHA1
>> +	select NVMEM
>>  	help
>>  	  Compute a persistent machine-specific id and store it to $global.machine_id.
>> -	  The id is a hash of device-specific information added via
>> -	  machine_id_set_hashable(). If multiple sources are available, the
>> -	  information provided by the last call prior to the late initcall
>> -	  set_machine_id() is used to generate the machine id from. Thus when
>> -	  updating barebox the machine id might change.
>> +	  The id is a hash of device-specific information added either via
>> +	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
>> +	  /chosen/barebox,machine-id device tree property.
>> +
>> +	  With machine_id_set_hashable(), the last call prior to the late initcall
>> +	  set_machine_id() willl be used to generate the machine id. This means
>> +	  updating barebox may change the machine id!
>>  
>>  	  global.bootm.provide_machine_id may be used to automatically set
>>  	  the linux.bootargs.machine_id global variable with a value of
>> diff --git a/common/machine_id.c b/common/machine_id.c
>> index 6480806cd287..fd2f0888a6cd 100644
>> --- a/common/machine_id.c
>> +++ b/common/machine_id.c
>> @@ -10,6 +10,9 @@
>>  #include <magicvar.h>
>>  #include <crypto/sha.h>
>>  #include <machine_id.h>
>> +#include <linux/err.h>
>> +#include <linux/nvmem-consumer.h>
>> +#include <of.h>
>>  
>>  #define MACHINE_ID_LENGTH 32
>>  
>> @@ -24,16 +27,49 @@ void machine_id_set_hashable(const void *hashable, size_t len)
>>  	__machine_id_hashable_length = len;
>>  }
>>  
>> +static const void *machine_id_get_hashable(size_t *len)
>> +{
>> +	struct device_node *np = of_get_machineidpath();
>> +	struct nvmem_cell *cell;
>> +	void *ret;
>> +
>> +	if (!np)
>> +		goto no_cell;
>> +
>> +	cell = of_nvmem_cell_get_from_node(np);
>> +	if (IS_ERR(cell)) {
>> +		pr_err("Invalid barebox,machine-id-path: %pe\n", cell);
>> +		goto no_cell;
>> +	}
>> +
>> +	ret = nvmem_cell_read(cell, len);
>> +	nvmem_cell_put(cell);
>> +	if (IS_ERR(ret)) {
>> +		pr_err("Couldn't read from barebox,machine-id-path: %pe\n", ret);
>> +		goto no_cell;
>> +	}
>> +
>> +	return ret;
>> +
>> +no_cell:
>> +	*len = __machine_id_hashable_length;
>> +	return __machine_id_hashable;
>> +}
>> +
>>  static int machine_id_set_globalvar(void)
>>  {
>>  	struct digest *digest = NULL;
>>  	unsigned char machine_id[SHA1_DIGEST_SIZE];
>>  	char hex_machine_id[MACHINE_ID_LENGTH];
>>  	char *env_machine_id;
>> +	const void *hashable;
>> +	size_t length;
>>  	int ret = 0;
>>  
>> +	hashable = machine_id_get_hashable(&length);
>> +
>>  	/* nothing to do if no hashable information provided */
>> -	if (!__machine_id_hashable)
>> +	if (!hashable)
>>  		goto out;
>>  
>>  	digest = digest_alloc_by_algo(HASH_ALGO_SHA1);
>> @@ -41,8 +77,7 @@ static int machine_id_set_globalvar(void)
>>  	if (ret)
>>  		goto out;
>>  
>> -	ret = digest_update(digest, __machine_id_hashable,
>> -			    __machine_id_hashable_length);
>> +	ret = digest_update(digest, hashable, length);
>>  	if (ret)
>>  		goto out;
>>  
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index 4e558e165063..980304a8078b 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -361,29 +361,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
>>  
>>  #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
>>  /**
>> - * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>> + * of_nvmem_cell_get_from_node() - Get a nvmem cell from cell device node
>>   *
>> - * @dev node: Device tree node that uses the nvmem cell
>> - * @id: nvmem cell name from nvmem-cell-names property.
>> + * @cell_np: Device tree node of the nvmem cell
>>   *
>>   * Return: Will be an ERR_PTR() on error or a valid pointer
>>   * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>>   * nvmem_cell_put().
>>   */
>> -struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>> -					    const char *name)
>> +struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
>>  {
>> -	struct device_node *cell_np, *nvmem_np;
>> +	struct device_node *nvmem_np;
>>  	struct nvmem_cell *cell;
>>  	struct nvmem_device *nvmem;
>>  	const __be32 *addr;
>> -	int rval, len, index;
>> -
>> -	index = of_property_match_string(np, "nvmem-cell-names", name);
>> -
>> -	cell_np = of_parse_phandle(np, "nvmem-cells", index);
>> -	if (!cell_np)
>> -		return ERR_PTR(-EINVAL);
>> +	int rval, len;
>>  
>>  	nvmem_np = of_get_parent(cell_np);
>>  	if (!nvmem_np)
>> @@ -445,6 +437,32 @@ err_mem:
>>  
>>  	return ERR_PTR(rval);
>>  }
>> +EXPORT_SYMBOL_GPL(of_nvmem_cell_get_from_node);
>> +
>> +/**
>> + * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>> + *
>> + * @dev node: Device tree node that uses the nvmem cell
>> + * @id: nvmem cell name from nvmem-cell-names property.
>> + *
>> + * Return: Will be an ERR_PTR() on error or a valid pointer
>> + * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>> + * nvmem_cell_put().
>> + */
>> +struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>> +					    const char *name)
>> +{
>> +	struct device_node *cell_np;
>> +	int index;
>> +
>> +	index = of_property_match_string(np, "nvmem-cell-names", name);
>> +
>> +	cell_np = of_parse_phandle(np, "nvmem-cells", index);
>> +	if (!cell_np)
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	return of_nvmem_cell_get_from_node(cell_np);
>> +}
>>  EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>>  #endif
> 
> Shouldn't these changes be part of a separate patch?

For barebox, I usually fold smaller changes into the commit that depends on them.
Sascha was ok with it so far. When the changes get bigger, I split them up,
but here I didn't deem this necessary.

> 
> Regards,
> Bastian
> 
>>  
>> diff --git a/drivers/of/base.c b/drivers/of/base.c
>> index 4104d7589305..a5b74707fc4f 100644
>> --- a/drivers/of/base.c
>> +++ b/drivers/of/base.c
>> @@ -2525,6 +2525,17 @@ int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate)
>>  	return true;
>>  }
>>  
>> +struct device_node *of_get_machineidpath(void)
>> +{
>> +	const char *name;
>> +
>> +	name = of_get_property(of_chosen, "barebox,machine-id-path", NULL);
>> +	if (!name)
>> +		return NULL;
>> +
>> +	return of_find_node_by_path_or_alias(NULL, name);
>> +}
>> +
>>  /**
>>   * of_add_initrd - add initrd properties to the devicetree
>>   * @root - the root node of the tree
>> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
>> index b979f23372a6..639a7ebbabae 100644
>> --- a/include/linux/nvmem-consumer.h
>> +++ b/include/linux/nvmem-consumer.h
>> @@ -122,11 +122,16 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
>>  #endif /* CONFIG_NVMEM */
>>  
>>  #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OFTREE)
>> +struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np);
>>  struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>  				     const char *name);
>>  struct nvmem_device *of_nvmem_device_get(struct device_node *np,
>>  					 const char *name);
>>  #else
>> +static inline struct nvmem_cell *of_nvmem_cell_get_from_node(struct device_node *cell_np)
>> +{
>> +	return ERR_PTR(-ENOSYS);
>> +}
>>  static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>  				     const char *name)
>>  {
>> diff --git a/include/of.h b/include/of.h
>> index d82790c0523e..677f48d0aba1 100644
>> --- a/include/of.h
>> +++ b/include/of.h
>> @@ -290,6 +290,7 @@ int of_fixup_partitions(struct device_node *np, struct cdev *cdev);
>>  int of_partitions_register_fixup(struct cdev *cdev);
>>  struct device_node *of_get_stdoutpath(unsigned int *);
>>  int of_device_is_stdout_path(struct device_d *dev, unsigned int *baudrate);
>> +struct device_node *of_get_machineidpath(void);
>>  const char *of_get_model(void);
>>  void *of_flatten_dtb(struct device_node *node);
>>  int of_add_memory(struct device_node *node, bool dump);
>> @@ -336,6 +337,11 @@ static inline int of_device_is_stdout_path(struct device_d *dev, unsigned int *b
>>  	return 0;
>>  }
>>  
>> +static inline struct device_node *of_get_machineidpath(void)
>> +{
>> +	return NULL;
>> +}
>> +
>>  static inline const char *of_get_model(void)
>>  {
>>  	return NULL;
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

* Re: [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable
  2021-06-28  9:50   ` Bastian Krause
@ 2021-06-28 10:12     ` Ahmad Fatoum
  0 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-28 10:12 UTC (permalink / raw)
  To: Bastian Krause, barebox



On 28.06.21 11:50, Bastian Krause wrote:
> On 6/28/21 8:40 AM, Ahmad Fatoum wrote:
>> The Kconfig symbol already warns users that barebox updates
>> that add new instances of machine_id_set_hashable may cause the machine
>> ID to change making the future less useful. Recent changes allow the
>> board DT to identify a specific nvmem cell to use for supplying a machine
>> ID, making the old way of drivers providing machine IDs and possibly
>> overriding each other no longer necessary or recommended.
> 
> I cannot parse the first sentence of the commit message.

There should've been a comma at the end of the first line.

Cheers,
Ahmad

> 
> Regards,
> Bastian
> 
>>
>> Make this explicit by allowing the old code to be disabled. As we only
>> have a single user now and won't accept any new ones, we can remove the
>> warning about it possibly changing after update.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  common/Kconfig        | 18 +++++++++++-------
>>  common/machine_id.c   | 11 ++++++++---
>>  drivers/nvmem/Kconfig |  1 +
>>  drivers/nvmem/ocotp.c |  7 ++++++-
>>  include/machine_id.h  | 16 ----------------
>>  5 files changed, 26 insertions(+), 27 deletions(-)
>>  delete mode 100644 include/machine_id.h
>>
>> diff --git a/common/Kconfig b/common/Kconfig
>> index a4a109f04f08..a2aa0b2568de 100644
>> --- a/common/Kconfig
>> +++ b/common/Kconfig
>> @@ -1069,13 +1069,11 @@ config MACHINE_ID
>>  	select NVMEM
>>  	help
>>  	  Compute a persistent machine-specific id and store it to $global.machine_id.
>> -	  The id is a hash of device-specific information added either via
>> -	  machine_id_set_hashable() or by hashing the nvmem cell referenced by the
>> -	  /chosen/barebox,machine-id device tree property.
>> -
>> -	  With machine_id_set_hashable(), the last call prior to the late initcall
>> -	  set_machine_id() willl be used to generate the machine id. This means
>> -	  updating barebox may change the machine id!
>> +	  The id is a hash of device-specific information. This information
>> +	  comes from the nvmem cell device tree node path described by the
>> +	  /chosen/barebox,machine-id-path property. As a special case, the i.MX6 OCOTP
>> +	  driver supplies the SoC UID as hashable for when /chosen/barebox,machine-id-path
>> +	  is not specified.
>>  
>>  	  global.bootm.provide_machine_id may be used to automatically set
>>  	  the linux.bootargs.machine_id global variable with a value of
>> @@ -1084,6 +1082,12 @@ config MACHINE_ID
>>  	  Note: if no hashable information is available no machine id will be passed
>>  	  to the kernel.
>>  
>> +config MACHINE_ID_LEGACY
>> +	bool
>> +	help
>> +	  Selected by i.MX OCOTP driver, so it can set the SoC UID as hashable.
>> +	  New platforms should use /chosen/barebox,machine-id-path instead.
>> +
>>  config SYSTEMD_OF_WATCHDOG
>>  	bool "inform devicetree-enabled kernel of used watchdog"
>>  	depends on WATCHDOG && OFTREE && FLEXIBLE_BOOTARGS
>> diff --git a/common/machine_id.c b/common/machine_id.c
>> index fd2f0888a6cd..8303c0b7aa87 100644
>> --- a/common/machine_id.c
>> +++ b/common/machine_id.c
>> @@ -9,23 +9,24 @@
>>  #include <globalvar.h>
>>  #include <magicvar.h>
>>  #include <crypto/sha.h>
>> -#include <machine_id.h>
>>  #include <linux/err.h>
>>  #include <linux/nvmem-consumer.h>
>>  #include <of.h>
>>  
>>  #define MACHINE_ID_LENGTH 32
>>  
>> +#ifdef CONFIG_MACHINE_ID_LEGACY
>>  static void *__machine_id_hashable;
>>  static size_t __machine_id_hashable_length;
>>  
>> -
>> +/* Shouldn't be called from new code */
>> +void machine_id_set_hashable(const void *hashable, size_t len);
>>  void machine_id_set_hashable(const void *hashable, size_t len)
>>  {
>> -
>>  	__machine_id_hashable = xmemdup(hashable, len);
>>  	__machine_id_hashable_length = len;
>>  }
>> +#endif
>>  
>>  static const void *machine_id_get_hashable(size_t *len)
>>  {
>> @@ -52,8 +53,12 @@ static const void *machine_id_get_hashable(size_t *len)
>>  	return ret;
>>  
>>  no_cell:
>> +#ifdef CONFIG_MACHINE_ID_LEGACY
>>  	*len = __machine_id_hashable_length;
>>  	return __machine_id_hashable;
>> +#else
>> +	return NULL;
>> +#endif
>>  }
>>  
>>  static int machine_id_set_globalvar(void)
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index 3781f7a839fc..08a5765573a8 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -25,6 +25,7 @@ config NVMEM_SNVS_LPGPR
>>  config IMX_OCOTP
>>  	tristate "i.MX6 On Chip OTP controller"
>>  	depends on ARCH_IMX6 || ARCH_VF610 || ARCH_IMX8M || ARCH_IMX7
>> +	select MACHINE_ID_LEGACY
>>  	depends on OFDEVICE
>>  	help
>>  	  This adds support for the i.MX6 On-Chip OTP controller. Currently the
>> diff --git a/drivers/nvmem/ocotp.c b/drivers/nvmem/ocotp.c
>> index b2fad3c68770..0b10e52a86ba 100644
>> --- a/drivers/nvmem/ocotp.c
>> +++ b/drivers/nvmem/ocotp.c
>> @@ -29,7 +29,6 @@
>>  #include <regmap.h>
>>  #include <linux/clk.h>
>>  #include <mach/ocotp.h>
>> -#include <machine_id.h>
>>  #include <mach/ocotp-fusemap.h>
>>  #include <linux/nvmem-provider.h>
>>  
>> @@ -689,6 +688,7 @@ static int imx_ocotp_read(void *ctx, unsigned offset, void *val, size_t bytes)
>>  
>>  static void imx_ocotp_set_unique_machine_id(void)
>>  {
>> +	extern void machine_id_set_hashable(const void *hashable, size_t len);
>>  	uint32_t unique_id_parts[UNIQUE_ID_NUM];
>>  	int i;
>>  
>> @@ -785,6 +785,11 @@ static int imx_ocotp_probe(struct device_d *dev)
>>  				  ethaddr->value, ethaddr);
>>  	}
>>  
>> +	/* Special case: new machine id providers should provide a nvmem cell
>> +	 * and reference its path via /chosen/barebox,machine-id-path.
>> +	 * For i.MX, we support the legacy way of the driver supplying the
>> +	 * hash instead
>> +	 */
>>  	if (IS_ENABLED(CONFIG_MACHINE_ID))
>>  		imx_ocotp_set_unique_machine_id();
>>  
>> diff --git a/include/machine_id.h b/include/machine_id.h
>> deleted file mode 100644
>> index 31d5e0bb2851..000000000000
>> --- a/include/machine_id.h
>> +++ /dev/null
>> @@ -1,16 +0,0 @@
>> -#ifndef __MACHINE_ID_H__
>> -#define __MACHINE_ID_H__
>> -
>> -#if IS_ENABLED(CONFIG_MACHINE_ID)
>> -
>> -void machine_id_set_hashable(const void *hashable, size_t len);
>> -
>> -#else
>> -
>> -static inline void machine_id_set_hashable(const void *hashable, size_t len)
>> -{
>> -}
>> -
>> -#endif /* CONFIG_MACHINE_ID */
>> -
>> -#endif  /* __MACHINE_ID_H__ */
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
  2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
                   ` (5 preceding siblings ...)
  2021-06-28  9:35 ` [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Bastian Krause
@ 2021-06-28 20:20 ` Sascha Hauer
       [not found] ` <CAMHeXxOT__KBUKG6GkNAEkqz4tMBBzuZ7OgnKa0_OX5hz-JEig@mail.gmail.com>
  7 siblings, 0 replies; 14+ messages in thread
From: Sascha Hauer @ 2021-06-28 20:20 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox, Bastian Krause, Uwe Kleine-König

Hi Ahmad,

On Mon, Jun 28, 2021 at 08:40:32AM +0200, Ahmad Fatoum wrote:
> The Kconfig option already warns that the current behavior of
> machine_id_set_hashable() overriding previous calls can lead to the
> machine-id changing over updates. We don't yet have this problem in
> practice, because the only two upstream users are for bsec and ocotp,
> which are efuse drivers for different SoCs. On the other hand, users
> may want to get the unique ID from an EEPROM and with deep probe
> support, the initcall ordering will be independent of the actual probe
> order.
> 
> Work around this issue by introducing a way for each board to explicitly
> reference a nvmem cell that should be hashed to produce the machine-id.
> 
> If no such device tree property is supplied, the last call to
> machine_id_set_hashable() will be used as before.
> 
> Cc: Bastian Krause <bst@pengutronix.de>
> Cc: Uwe Kleine-König <ukl@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  common/Kconfig                 | 13 ++++++----
>  common/machine_id.c            | 41 ++++++++++++++++++++++++++++---
>  drivers/nvmem/core.c           | 44 ++++++++++++++++++++++++----------
>  drivers/of/base.c              | 11 +++++++++
>  include/linux/nvmem-consumer.h |  5 ++++
>  include/of.h                   |  6 +++++

This lacks a change in Documentation/devicetree/bindings/ :)

Regards,
  Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox


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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
       [not found] ` <CAMHeXxOT__KBUKG6GkNAEkqz4tMBBzuZ7OgnKa0_OX5hz-JEig@mail.gmail.com>
@ 2021-06-30 10:27   ` Ahmad Fatoum
  2021-06-30 20:13     ` Trent Piepho
  0 siblings, 1 reply; 14+ messages in thread
From: Ahmad Fatoum @ 2021-06-30 10:27 UTC (permalink / raw)
  To: Trent Piepho, barebox

Hello Trent,

(Cc'ing mailing list)

On 28.06.21 21:50, Trent Piepho wrote:
> On Sun, Jun 27, 2021 at 11:41 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>> The Kconfig option already warns that the current behavior of
>> machine_id_set_hashable() overriding previous calls can lead to the
>> machine-id changing over updates. We don't yet have this problem in
>> practice, because the only two upstream users are for bsec and ocotp,
>> which are efuse drivers for different SoCs. On the other hand, users
>> may want to get the unique ID from an EEPROM and with deep probe
>> support, the initcall ordering will be independent of the actual probe
>> order.
> 
> On a board I did before Barebox had machine-id support, we used two
> sources of serial number to generate the machine id.  One was the imx7
> unique id and the other was a serial number in a i2c security chip.
> 
> The imx id is predictable, so even hashed one can predict the
> machine-id exactly. 

We happen to have stm32mp1 twins with consecutive MAC addresses.
I compared their serial numbers and while they clearly didn't start
at zero, all bytes were the same, except for two nibbles that were
one apart. So yes, if the i.MX UID follows a similar scheme, you
may be able to guess the machine-id of other devices in the same
batch if you already have the machine-id of one of them.

> We didn't really like that, so we combined two
 sources.

Is your issue one of privacy or security?

My understanding is that the machine id is not disclosed as matter
of privacy, so it's harder to track a device by manner of the unique
IDs embedded in its outgoing communication.

> It seems like there is no way to do that with this design?

You can write a driver that collects multiple sources and offers
a single NVMEM cell for consumption.

> I think it could be done if there was a function to add hash input,
> which board code could call.  Keep the pools separate and have a
> defined order.  I.e., /chosen/barebox,machine-id is first, then
> machine_id_add_hashable() is next.

If we restrict the new machine_id_add_hashable() for board use that
would work, yes. I don't like it from a design perspective though:
A user would expect a machine-id-path property to point at all info
used for determining the machine-id, not some of them.

> Or /chosen/barebox,machine-id could be a _list_ of paths, to be used
> in order.  But that requires a nvmem driver for each source. 

That's fine by me and could be added in future. I can add a property
size check, so we leave open this avenue without breaking users.

The root device tree node already has a device in barebox, so board
could use that to offer custom info.

> The security chip wouldn't have worked for a nvmem driver.

Why not? Check out nvmem-rmem with just exports a memory region
as read-only nvmem device. You can do likewise. There are also
helpers to get a nvmem device out of a (i2c) regmap.

> I'll also point out that just hashing the data is not a good idea to
> make a UUID.  Anyone who hashes the imx unique id will get the exact
> same UUID.  So it is not very universally unique!

The i.MX unique ID is unique across i.MX processors. Yes, it would
collide with an attacker that guess the ID, but is that really
a threat? Anyhow, there are boards using it like this already in the
field. So this won't change, but for the new binding introduced here,
I can address issues that are raised.
> Also consider if I give every board a unique sequential serial number
> from eeprom and use that.  Then someone else makes a completely
> different board and they also use sequential serial numbers unique for
> their boards.  But our serial numbers are the same as theirs, so we
> have a UUID collision.
> 
> This is already a known issue when generating UUIDs that are not
> purely random.  See RFC4122 §4.3 for generating UUIDs in a namespace.
> This is what I used.  The namespace would be barebox + board type, so
> that unique UUIDs are created when different types of boards use the
> same serial number and also "barebox", so when a single board creates
> different UUIDs for different reasons but they are all based on the
> serial number they UUIDs also still unique
Users can do that, barebox will hashh it to get the format the OS expects.
It's probably a good idea to hint at RFC4122 for users interested in
generating their own material for use with the machine-id.

Cheers,
Ahmad


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
  2021-06-30 10:27   ` Ahmad Fatoum
@ 2021-06-30 20:13     ` Trent Piepho
  2021-09-15 10:55       ` Ahmad Fatoum
  0 siblings, 1 reply; 14+ messages in thread
From: Trent Piepho @ 2021-06-30 20:13 UTC (permalink / raw)
  To: Ahmad Fatoum; +Cc: barebox

On Wed, Jun 30, 2021 at 3:27 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> On 28.06.21 21:50, Trent Piepho wrote:
> > On Sun, Jun 27, 2021 at 11:41 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
> >
> > On a board I did before Barebox had machine-id support, we used two
> > sources of serial number to generate the machine id.  One was the imx7
> > unique id and the other was a serial number in a i2c security chip.
> >
> > The imx id is predictable, so even hashed one can predict the
> > machine-id exactly.
>
> We happen to have stm32mp1 twins with consecutive MAC addresses.
> I compared their serial numbers and while they clearly didn't start
> at zero, all bytes were the same, except for two nibbles that were
> one apart. So yes, if the i.MX UID follows a similar scheme, you
> may be able to guess the machine-id of other devices in the same
> batch if you already have the machine-id of one of them.
>
> > We didn't really like that, so we combined two
>  sources.
>
> Is your issue one of privacy or security?

Both.  We didn't like it being guessable for fishing attempts.
Security was more on the cloud side, having the machine-ids guessable
was not a good idea.  Yes, machine id is not to be used for access
control, it is not a password.  But one does not want an attacker to
have a list of every valid username even if the password is still
secret.

> My understanding is that the machine id is not disclosed as matter
> of privacy, so it's harder to track a device by manner of the unique
> IDs embedded in its outgoing communication.
>
> > It seems like there is no way to do that with this design?
>
> You can write a driver that collects multiple sources and offers
> a single NVMEM cell for consumption.

I suppose this could be done, but it certainly seems complex.  I think
one would need:
Write code in board init function to get imx id, data from additional
chip, put in global buffer.
Add barebox specific device tree node, compatible =
"mycompany,myboard-unique-data"
Write small nvmem driver that exports the global buffer as nvmem device.

I suppose the latter driver could be common if needing to create nvmem
devices to hold data becomes commonly needed.

> > I think it could be done if there was a function to add hash input,
> > which board code could call.  Keep the pools separate and have a
> > defined order.  I.e., /chosen/barebox,machine-id is first, then
> > machine_id_add_hashable() is next.
>
> If we restrict the new machine_id_add_hashable() for board use that
> would work, yes. I don't like it from a design perspective though:

Yes, only board code should call that.  It must be unique and
invariant to the board and no individual driver knows enough to do
that.

> A user would expect a machine-id-path property to point at all info
> used for determining the machine-id, not some of them.

One could have a special name in the path, "internal" or something,
that indicates data that code in Barebox will create.

> > Or /chosen/barebox,machine-id could be a _list_ of paths, to be used
> > in order.  But that requires a nvmem driver for each source.
>
> That's fine by me and could be added in future. I can add a property
> size check, so we leave open this avenue without breaking users.
>
> The root device tree node already has a device in barebox, so board
> could use that to offer custom info.

If a property was created that simply contained the data directly,
I.e. of_set_property(root, "extra-id-data", data, 8),
would there be a way for barebox,machine-id to point to it?

> > The security chip wouldn't have worked for a nvmem driver.
>
> Why not? Check out nvmem-rmem with just exports a memory region
> as read-only nvmem device. You can do likewise. There are also
> helpers to get a nvmem device out of a (i2c) regmap.

To get the id, one needs to construct a command and then parse the
response format to extract the data.  It is not as simple as some
register contents.

> > I'll also point out that just hashing the data is not a good idea to
> > make a UUID.  Anyone who hashes the imx unique id will get the exact
> > same UUID.  So it is not very universally unique!
>
> The i.MX unique ID is unique across i.MX processors. Yes, it would
> collide with an attacker that guess the ID, but is that really
> a threat? Anyhow, there are boards using it like this already in the

It comes up if some other software on the same board creates a UUID by
hashing the imx id.  Anything that does this will get the same UUID.
Suppose the board is not imx and has no unique built-in id other than
a MAC address.  Anyone who hashes the mac address gets the same UUID
and they collide.  It is not a security issue exactly, but a failure
of the universally unique property of the UUID.

> field. So this won't change, but for the new binding introduced here,
> I can address issues that are raised.

> > This is already a known issue when generating UUIDs that are not
> > purely random.  See RFC4122 §4.3 for generating UUIDs in a namespace.

> Users can do that, barebox will hashh it to get the format the OS expects.
> It's probably a good idea to hint at RFC4122 for users interested in
> generating their own material for use with the machine-id.

I don't know what you mean by the format the OS expects.  The output
of the §4.3 algorithm is a standard UUID.  It works perfectly well to
pass it as a machine id and then systemd will use it as it is.  Since
2011, when systemd generates a machine id it follows RFC4422 §4.4 for
random UUIDs.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

* Re: [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override
  2021-06-30 20:13     ` Trent Piepho
@ 2021-09-15 10:55       ` Ahmad Fatoum
  0 siblings, 0 replies; 14+ messages in thread
From: Ahmad Fatoum @ 2021-09-15 10:55 UTC (permalink / raw)
  To: Trent Piepho; +Cc: barebox

Hi,

On 30.06.21 22:13, Trent Piepho wrote:
> On Wed, Jun 30, 2021 at 3:27 AM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>> On 28.06.21 21:50, Trent Piepho wrote:
>>> On Sun, Jun 27, 2021 at 11:41 PM Ahmad Fatoum <a.fatoum@pengutronix.de> wrote:
>>>
>>> On a board I did before Barebox had machine-id support, we used two
>>> sources of serial number to generate the machine id.  One was the imx7
>>> unique id and the other was a serial number in a i2c security chip.
>>>
>>> The imx id is predictable, so even hashed one can predict the
>>> machine-id exactly.
>>
>> We happen to have stm32mp1 twins with consecutive MAC addresses.
>> I compared their serial numbers and while they clearly didn't start
>> at zero, all bytes were the same, except for two nibbles that were
>> one apart. So yes, if the i.MX UID follows a similar scheme, you
>> may be able to guess the machine-id of other devices in the same
>> batch if you already have the machine-id of one of them.
>>
>>> We didn't really like that, so we combined two
>>  sources.
>>
>> Is your issue one of privacy or security?
> 
> Both.  We didn't like it being guessable for fishing attempts.
> Security was more on the cloud side, having the machine-ids guessable
> was not a good idea.  Yes, machine id is not to be used for access
> control, it is not a password.  But one does not want an attacker to
> have a list of every valid username even if the password is still
> secret.

Fair enough.

>> My understanding is that the machine id is not disclosed as matter
>> of privacy, so it's harder to track a device by manner of the unique
>> IDs embedded in its outgoing communication.
>>
>>> It seems like there is no way to do that with this design?
>>
>> You can write a driver that collects multiple sources and offers
>> a single NVMEM cell for consumption.
> 
> I suppose this could be done, but it certainly seems complex.  I think
> one would need:
> Write code in board init function to get imx id, data from additional
> chip, put in global buffer.
> Add barebox specific device tree node, compatible =
> "mycompany,myboard-unique-data"
> Write small nvmem driver that exports the global buffer as nvmem device.
> 
> I suppose the latter driver could be common if needing to create nvmem
> devices to hold data becomes commonly needed.

Yes. I think using a nvmem cell as an interface here is a good
trade off. It works for most and it's straight forward to extend
in future.
 
>> A user would expect a machine-id-path property to point at all info
>> used for determining the machine-id, not some of them.
> 
> One could have a special name in the path, "internal" or something,
> that indicates data that code in Barebox will create.
> 
>>> Or /chosen/barebox,machine-id could be a _list_ of paths, to be used
>>> in order.  But that requires a nvmem driver for each source.
>>
>> That's fine by me and could be added in future. I can add a property
>> size check, so we leave open this avenue without breaking users.
>>
>> The root device tree node already has a device in barebox, so board
>> could use that to offer custom info.
> 
> If a property was created that simply contained the data directly,
> I.e. of_set_property(root, "extra-id-data", data, 8),
> would there be a way for barebox,machine-id to point to it?

Nope. But who would fix up such a property? If you get data
from a previous boot stage, you could put a nvmem-romem on top.

>>> The security chip wouldn't have worked for a nvmem driver.
>>
>> Why not? Check out nvmem-rmem with just exports a memory region
>> as read-only nvmem device. You can do likewise. There are also
>> helpers to get a nvmem device out of a (i2c) regmap.
> 
> To get the id, one needs to construct a command and then parse the
> response format to extract the data.  It is not as simple as some
> register contents.
> 
>>> I'll also point out that just hashing the data is not a good idea to
>>> make a UUID.  Anyone who hashes the imx unique id will get the exact
>>> same UUID.  So it is not very universally unique!
>>
>> The i.MX unique ID is unique across i.MX processors. Yes, it would
>> collide with an attacker that guess the ID, but is that really
>> a threat? Anyhow, there are boards using it like this already in the
> 
> It comes up if some other software on the same board creates a UUID by
> hashing the imx id.  Anything that does this will get the same UUID.
> Suppose the board is not imx and has no unique built-in id other than
> a MAC address.  Anyone who hashes the mac address gets the same UUID
> and they collide.  It is not a security issue exactly, but a failure
> of the universally unique property of the UUID.

The alternative is not to have a default at all. So we provide
a default that works for most and if you need more than that,
you can extend it.

>> field. So this won't change, but for the new binding introduced here,
>> I can address issues that are raised.
> 
>>> This is already a known issue when generating UUIDs that are not
>>> purely random.  See RFC4122 §4.3 for generating UUIDs in a namespace.
> 
>> Users can do that, barebox will hashh it to get the format the OS expects.
>> It's probably a good idea to hint at RFC4122 for users interested in
>> generating their own material for use with the machine-id.
> 
> I don't know what you mean by the format the OS expects.  The output
> of the §4.3 algorithm is a standard UUID.  It works perfectly well to
> pass it as a machine id and then systemd will use it as it is.  Since
> 2011, when systemd generates a machine id it follows RFC4422 §4.4 for
> random UUIDs.

systemd expects a 32 character lower case ID that's not all zeroes.
Newer systemd versions apparently generate machine IDs that qualify
as v4 UUIDs, but barebox doesn't and I just want to streamline the
existing implementation here. I don't intend to make the machine-id
UUID-compatible (although I don't mind extending the functionality
into that direction).

Cheers,
Ahmad 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

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

end of thread, other threads:[~2021-09-15 10:57 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-28  6:40 [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 2/5] ARM: stm32mp: migrate to barebox,machine-id-path Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 3/5] common: machine_id: deprecate machine_id_set_hashable Ahmad Fatoum
2021-06-28  9:50   ` Bastian Krause
2021-06-28 10:12     ` Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 4/5] sandbox: dts: populate $global.machine_id Ahmad Fatoum
2021-06-28  6:40 ` [PATCH 5/5] ARM: dts: stm32mp: retire barebox, provide-mac-address in favor of NVMEM Ahmad Fatoum
2021-06-28  9:28 ` [PATCH 1/5] common: machine_id: support /chosen/barebox,machine-id-path override Ahmad Fatoum
2021-06-28  9:35 ` [PATCH 1/5] common: machine_id: support /chosen/barebox, machine-id-path override Bastian Krause
2021-06-28 10:11   ` Ahmad Fatoum
2021-06-28 20:20 ` Sascha Hauer
     [not found] ` <CAMHeXxOT__KBUKG6GkNAEkqz4tMBBzuZ7OgnKa0_OX5hz-JEig@mail.gmail.com>
2021-06-30 10:27   ` Ahmad Fatoum
2021-06-30 20:13     ` Trent Piepho
2021-09-15 10:55       ` Ahmad Fatoum

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox