[PATCH 1/2] drivers/of: validate live-tree string properties before string use

[PATCH 1/2] drivers/of: validate live-tree string properties before string use

[Pengpeng Hou]

`populate_properties()` stores live-tree property values as raw byte
sequences plus a separate `length`. They are not globally guaranteed to
be NUL-terminated.

`of_prop_next_string()` currently advances through string-list
properties with `strlen()`, `__of_node_is_type()` compares raw
`device_type` bytes with `strcmp()`, `__of_device_is_status()` compares
raw `status` bytes with `strcmp()`/`strncmp()`, and
`of_alias_from_compatible()` treats the first `compatible` entry as a
NUL-terminated string.

Validate these strings within their property bounds before treating
them as C strings. In particular, reject malformed string-list entries
whose next string is not terminated before `of_prop_next_string()`
returns it.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

---
drivers/of/base.c     | 39 +++++++++++++++++++++++----------------
drivers/of/property.c | 30 +++++++++++++++++++++++++-----
 2 files changed, 48 insertions(+), 21 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 57420806c1a2..3c6af4051ad3 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -82,7 +82,14 @@ EXPORT_SYMBOL(of_node_name_prefix);
 
 static bool __of_node_is_type(const struct device_node *np, const char *type)
 {
-	const char *match = __of_get_property(np, "device_type", NULL);
+	const char *match;
+	int matchlen;
+
+	match = __of_get_property(np, "device_type", &matchlen);
+	if (!match || matchlen <= 0)
+		return false;
+	if (strnlen(match, matchlen) >= matchlen)
+		return false;
 
 	return np && match && type && !strcmp(match, type);
 }
@@ -491,22 +498,22 @@ static bool __of_device_is_status(const struct device_node *device,
 		return false;
 
 	status = __of_get_property(device, "status", &statlen);
-	if (status == NULL)
+	if (!status || statlen <= 0)
+		return false;
+	if (strnlen(status, statlen) >= statlen)
 		return false;
 
-	if (statlen > 0) {
-		while (*strings) {
-			unsigned int len = strlen(*strings);
+	while (*strings) {
+		unsigned int len = strlen(*strings);
 
-			if ((*strings)[len - 1] == '-') {
-				if (!strncmp(status, *strings, len))
-					return true;
-			} else {
-				if (!strcmp(status, *strings))
-					return true;
-			}
-			strings++;
+		if ((*strings)[len - 1] == '-') {
+			if (!strncmp(status, *strings, len))
+				return true;
+		} else {
+			if (!strcmp(status, *strings))
+				return true;
 		}
+		strings++;
 	}
 
 	return false;
@@ -1217,10 +1224,10 @@ EXPORT_SYMBOL(of_find_matching_node_and_match);
 int of_alias_from_compatible(const struct device_node *node, char *alias, int len)
 {
 	const char *compatible, *p;
-	int cplen;
+	int ret;
 
-	compatible = of_get_property(node, "compatible", &cplen);
-	if (!compatible || strlen(compatible) > cplen)
+	ret = of_property_read_string_index(node, "compatible", 0, &compatible);
+	if (ret)
 		return -ENODEV;
 	p = strchr(compatible, ',');
 	strscpy(alias, p ? p + 1 : compatible, len);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 50d95d512bf5..edbc7a95aa4c 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -648,16 +648,36 @@ EXPORT_SYMBOL_GPL(of_prop_next_u32);
 
 const char *of_prop_next_string(const struct property *prop, const char *cur)
 {
-	const void *curv = cur;
+	const char *curv = cur;
+	const char *end;
+	size_t len;
 
-	if (!prop)
+	if (!prop || !prop->value || !prop->length)
 		return NULL;
 
-	if (!cur)
+	end = prop->value + prop->length;
+
+	if (!cur) {
+		len = strnlen(prop->value, prop->length);
+		if (len >= prop->length)
+			return NULL;
+
 		return prop->value;
+	}
 
-	curv += strlen(cur) + 1;
-	if (curv >= prop->value + prop->length)
+	if (cur < (const char *)prop->value || cur >= end)
+		return NULL;
+
+	len = strnlen(cur, end - cur);
+	if (len >= end - cur)
+		return NULL;
+
+	curv += len + 1;
+	if (curv >= end)
+		return NULL;
+
+	len = strnlen(curv, end - curv);
+	if (len >= end - curv)
 		return NULL;
 
 	return curv;
-- 
2.50.1 (Apple Git-155)

[PATCH 2/2] drivers/of: validate status properties in reconfig state changes

[Pengpeng Hou]

Live-tree reconfiguration properties also carry raw values plus explicit
lengths. `of_reconfig_get_state_change()` currently treats `status`
property values as NUL-terminated strings and feeds them straight into
`strcmp()`.

Factor the `"okay"` / `"ok"` check out into a helper that first verifies
that the property contains a bounded C string within `prop->length`.
Malformed `status` updates should be treated as not enabling the node.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>

---
drivers/of/dynamic.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1a06175def37..efee59ed371a 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -74,6 +74,20 @@ static const char *action_names[] = {
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
 
+static bool of_property_status_ok(const struct property *prop)
+{
+	const char *status;
+
+	if (!prop || !prop->value || prop->length <= 0)
+		return false;
+
+	status = prop->value;
+	if (strnlen(status, prop->length) >= prop->length)
+		return false;
+
+	return !strcmp(status, "okay") || !strcmp(status, "ok");
+}
+
 #define _do_print(func, prefix, action, node, prop, ...) ({	\
 	func("changeset: " prefix "%-15s %pOF%s%s\n",		\
 	     ##__VA_ARGS__, action_names[action], node,		\
@@ -135,11 +149,9 @@ int of_reconfig_get_state_change(unsigned long action, struct of_reconfig_data *
 
 	if (prop && !strcmp(prop->name, "status")) {
 		is_status = 1;
-		status_state = !strcmp(prop->value, "okay") ||
-			       !strcmp(prop->value, "ok");
+		status_state = of_property_status_ok(prop);
 		if (old_prop)
-			old_status_state = !strcmp(old_prop->value, "okay") ||
-					   !strcmp(old_prop->value, "ok");
+			old_status_state = of_property_status_ok(old_prop);
 	}
 
 	switch (action) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 1/2] drivers/of: validate live-tree string properties before string use

[Rob Herring]

On Fri, Apr 03, 2026 at 03:32:30PM +0800, Pengpeng Hou wrote:
> `populate_properties()` stores live-tree property values as raw byte
> sequences plus a separate `length`. They are not globally guaranteed to
> be NUL-terminated.
> 
> `of_prop_next_string()` currently advances through string-list
> properties with `strlen()`, `__of_node_is_type()` compares raw
> `device_type` bytes with `strcmp()`, `__of_device_is_status()` compares
> raw `status` bytes with `strcmp()`/`strncmp()`, and
> `of_alias_from_compatible()` treats the first `compatible` entry as a
> NUL-terminated string.
> 
> Validate these strings within their property bounds before treating
> them as C strings. In particular, reject malformed string-list entries
> whose next string is not terminated before `of_prop_next_string()`
> returns it.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> 
> ---
> drivers/of/base.c     | 39 +++++++++++++++++++++++----------------
> drivers/of/property.c | 30 +++++++++++++++++++++++++-----
>  2 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 57420806c1a2..3c6af4051ad3 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -82,7 +82,14 @@ EXPORT_SYMBOL(of_node_name_prefix);
>  
>  static bool __of_node_is_type(const struct device_node *np, const char *type)
>  {
> -	const char *match = __of_get_property(np, "device_type", NULL);
> +	const char *match;
> +	int matchlen;
> +
> +	match = __of_get_property(np, "device_type", &matchlen);
> +	if (!match || matchlen <= 0)
> +		return false;
> +	if (strnlen(match, matchlen) >= matchlen)
> +		return false;

Can't we use of_property_match_string() instead?

>  
>  	return np && match && type && !strcmp(match, type);
>  }
> @@ -491,22 +498,22 @@ static bool __of_device_is_status(const struct device_node *device,
>  		return false;
>  
>  	status = __of_get_property(device, "status", &statlen);
> -	if (status == NULL)
> +	if (!status || statlen <= 0)
> +		return false;
> +	if (strnlen(status, statlen) >= statlen)
>  		return false;
>  
> -	if (statlen > 0) {
> -		while (*strings) {
> -			unsigned int len = strlen(*strings);
> +	while (*strings) {
> +		unsigned int len = strlen(*strings);
>  
> -			if ((*strings)[len - 1] == '-') {
> -				if (!strncmp(status, *strings, len))
> -					return true;
> -			} else {
> -				if (!strcmp(status, *strings))
> -					return true;
> -			}
> -			strings++;
> +		if ((*strings)[len - 1] == '-') {
> +			if (!strncmp(status, *strings, len))
> +				return true;
> +		} else {
> +			if (!strcmp(status, *strings))
> +				return true;
>  		}
> +		strings++;
>  	}
>  
>  	return false;
> @@ -1217,10 +1224,10 @@ EXPORT_SYMBOL(of_find_matching_node_and_match);
>  int of_alias_from_compatible(const struct device_node *node, char *alias, int len)
>  {
>  	const char *compatible, *p;
> -	int cplen;
> +	int ret;
>  
> -	compatible = of_get_property(node, "compatible", &cplen);
> -	if (!compatible || strlen(compatible) > cplen)
> +	ret = of_property_read_string_index(node, "compatible", 0, &compatible);
> +	if (ret)
>  		return -ENODEV;
>  	p = strchr(compatible, ',');
>  	strscpy(alias, p ? p + 1 : compatible, len);
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 50d95d512bf5..edbc7a95aa4c 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -648,16 +648,36 @@ EXPORT_SYMBOL_GPL(of_prop_next_u32);
>  
>  const char *of_prop_next_string(const struct property *prop, const char *cur)
>  {
> -	const void *curv = cur;
> +	const char *curv = cur;
> +	const char *end;
> +	size_t len;
>  
> -	if (!prop)
> +	if (!prop || !prop->value || !prop->length)
>  		return NULL;
>  
> -	if (!cur)
> +	end = prop->value + prop->length;
> +
> +	if (!cur) {
> +		len = strnlen(prop->value, prop->length);
> +		if (len >= prop->length)
> +			return NULL;
> +
>  		return prop->value;

You return here, but cur is still NULL. So we never advance. We should 
have a test case in the unit test for this. If not, please add one.

> +	}
>  
> -	curv += strlen(cur) + 1;
> -	if (curv >= prop->value + prop->length)
> +	if (cur < (const char *)prop->value || cur >= end)
> +		return NULL;
> +
> +	len = strnlen(cur, end - cur);
> +	if (len >= end - cur)
> +		return NULL;
> +
> +	curv += len + 1;
> +	if (curv >= end)
> +		return NULL;
> +
> +	len = strnlen(curv, end - curv);

Can we make this so we're not doing strnlen() on each string twice. If 
return the current string, then 'cur' can point to the next string.


> +	if (len >= end - curv)
>  		return NULL;
>  
>  	return curv;
> -- 
> 2.50.1 (Apple Git-155)
> 
> 

Re: [PATCH 1/2] drivers/of: validate live-tree string properties before string use

[Pengpeng Hou]

Hi Rob,

Thanks, I'll respin this.

I'll switch the `device_type` match to `of_property_match_string()`,
rework `of_prop_next_string()` so the `cur == NULL` case flows through a
cleaner iterator path without validating each returned string twice, and
add a unittest for that first-iteration case.

Thanks,
Pengpeng

[PATCH v2 1/2] drivers/of: validate live-tree string properties before string use

[Pengpeng Hou]

`populate_properties()` stores live-tree property values as raw byte
sequences plus a separate `length`. They are not globally guaranteed to
be NUL-terminated.

`of_prop_next_string()` iterates string-list properties by walking raw
bytes, `__of_node_is_type()` checks `device_type`,
`__of_device_is_status()` checks `status`, and
`of_alias_from_compatible()` reads the first `compatible` entry. These
paths must validate that the relevant string fits within the property
bounds before they hand it to C string helpers.

Validate these live-tree string properties within their declared bounds.
In particular, make `of_prop_next_string()` reject malformed entries
before returning them, use `of_property_match_string()` for
`device_type`, and add unit coverage for malformed first and trailing
string-list entries.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- use of_property_match_string() for device_type as suggested by
  Rob Herring
- rework of_prop_next_string() so the first returned string is validated
  through the same bounded path
- add of_unittest_property_string() coverage for malformed first and
  trailing string-list entries

 drivers/of/base.c     | 36 ++++++++++++----------
 drivers/of/property.c | 27 +++++++++++++++++-----
 drivers/of/unittest.c | 32 ++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 23 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 57420806c1a2..96e4d7a7d5b8 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -82,9 +82,10 @@ EXPORT_SYMBOL(of_node_name_prefix);
 
 static bool __of_node_is_type(const struct device_node *np, const char *type)
 {
-	const char *match = __of_get_property(np, "device_type", NULL);
+	if (!np || !type)
+		return false;
 
-	return np && match && type && !strcmp(match, type);
+	return of_property_match_string(np, "device_type", type) == 0;
 }
 
 #define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \
@@ -491,22 +492,22 @@ static bool __of_device_is_status(const struct device_node *device,
 		return false;
 
 	status = __of_get_property(device, "status", &statlen);
-	if (status == NULL)
+	if (!status || statlen <= 0)
+		return false;
+	if (strnlen(status, statlen) >= statlen)
 		return false;
 
-	if (statlen > 0) {
-		while (*strings) {
-			unsigned int len = strlen(*strings);
+	while (*strings) {
+		unsigned int len = strlen(*strings);
 
-			if ((*strings)[len - 1] == '-') {
-				if (!strncmp(status, *strings, len))
-					return true;
-			} else {
-				if (!strcmp(status, *strings))
-					return true;
-			}
-			strings++;
+		if ((*strings)[len - 1] == '-') {
+			if (!strncmp(status, *strings, len))
+				return true;
+		} else {
+			if (!strcmp(status, *strings))
+				return true;
 		}
+		strings++;
 	}
 
 	return false;
@@ -1217,10 +1218,11 @@ EXPORT_SYMBOL(of_find_matching_node_and_match);
 int of_alias_from_compatible(const struct device_node *node, char *alias, int len)
 {
 	const char *compatible, *p;
-	int cplen;
+	int ret;
 
-	compatible = of_get_property(node, "compatible", &cplen);
-	if (!compatible || strlen(compatible) > cplen)
+	ret = of_property_read_string_index(node, "compatible", 0,
+					    &compatible);
+	if (ret)
 		return -ENODEV;
 	p = strchr(compatible, ',');
 	strscpy(alias, p ? p + 1 : compatible, len);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 50d95d512bf5..e97bfe357808 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -648,16 +648,31 @@ EXPORT_SYMBOL_GPL(of_prop_next_u32);
 
 const char *of_prop_next_string(const struct property *prop, const char *cur)
 {
-	const void *curv = cur;
+	const char *curv;
+	const char *end;
+	size_t len;
 
-	if (!prop)
+	if (!prop || !prop->value || !prop->length)
 		return NULL;
 
-	if (!cur)
-		return prop->value;
+	curv = cur ? cur : prop->value;
+	end = prop->value + prop->length;
 
-	curv += strlen(cur) + 1;
-	if (curv >= prop->value + prop->length)
+	if (curv < (const char *)prop->value || curv >= end)
+		return NULL;
+
+	if (cur) {
+		len = strnlen(curv, end - curv);
+		if (len >= end - curv)
+			return NULL;
+
+		curv += len + 1;
+		if (curv >= end)
+			return NULL;
+	}
+
+	len = strnlen(curv, end - curv);
+	if (len >= end - curv)
 		return NULL;
 
 	return curv;
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 29402958f11c..ee53363dfa84 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -713,6 +713,7 @@ static void __init of_unittest_parse_phandle_with_args_map(void)
 static void __init of_unittest_property_string(void)
 {
 	const char *strings[4];
+	const struct property *prop;
 	struct device_node *np;
 	int rc;
 
@@ -789,6 +790,37 @@ static void __init of_unittest_property_string(void)
 	strings[1] = NULL;
 	rc = of_property_read_string_array(np, "phandle-list-names", strings, 1);
 	unittest(rc == 1 && strings[1] == NULL, "Overwrote end of string array; rc=%i, str='%s'\n", rc, strings[1]);
+
+	/* of_prop_next_string() tests */
+	prop = of_find_property(np, "phandle-list-names", NULL);
+	strings[0] = of_prop_next_string(prop, NULL);
+	unittest(strings[0] && !strcmp(strings[0], "first"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(strings[0] && !strcmp(strings[0], "second"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(strings[0] && !strcmp(strings[0], "third"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(!strings[0],
+		 "of_prop_next_string() should return NULL at end of list\n");
+
+	prop = of_find_property(np, "unterminated-string", NULL);
+	strings[0] = of_prop_next_string(prop, NULL);
+	unittest(!strings[0],
+		 "of_prop_next_string() should reject unterminated first string\n");
+
+	prop = of_find_property(np, "unterminated-string-list", NULL);
+	strings[0] = of_prop_next_string(prop, NULL);
+	unittest(strings[0] && !strcmp(strings[0], "first"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(strings[0] && !strcmp(strings[0], "second"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(!strings[0],
+		 "of_prop_next_string() should reject unterminated trailing string\n");
 }
 
 #define propcmp(p1, p2) (((p1)->length == (p2)->length) && \
-- 
2.50.1 (Apple Git-155)

[PATCH v2 2/2] drivers/of: validate status properties in reconfig state changes

[Pengpeng Hou]

Live-tree reconfiguration properties also carry raw values plus explicit
lengths. `of_reconfig_get_state_change()` currently treats `status`
property values as NUL-terminated strings and feeds them straight into
`strcmp()`.

Factor the `"okay"` / `"ok"` check out into a helper that first verifies
that the property contains a bounded C string within `prop->length`.
Malformed `status` updates should be treated as not enabling the node.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v1:
- no change; carried in v2 because patch 1/2 was reworked

 drivers/of/dynamic.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1a06175def37..efee59ed371a 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -74,6 +74,20 @@ static const char *action_names[] = {
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
 
+static bool of_property_status_ok(const struct property *prop)
+{
+	const char *status;
+
+	if (!prop || !prop->value || prop->length <= 0)
+		return false;
+
+	status = prop->value;
+	if (strnlen(status, prop->length) >= prop->length)
+		return false;
+
+	return !strcmp(status, "okay") || !strcmp(status, "ok");
+}
+
 #define _do_print(func, prefix, action, node, prop, ...) ({	\
 	func("changeset: " prefix "%-15s %pOF%s%s\n",		\
 	     ##__VA_ARGS__, action_names[action], node,		\
@@ -135,11 +149,9 @@ int of_reconfig_get_state_change(unsigned long action, struct of_reconfig_data *
 
 	if (prop && !strcmp(prop->name, "status")) {
 		is_status = 1;
-		status_state = !strcmp(prop->value, "okay") ||
-			       !strcmp(prop->value, "ok");
+		status_state = of_property_status_ok(prop);
 		if (old_prop)
-			old_status_state = !strcmp(old_prop->value, "okay") ||
-					   !strcmp(old_prop->value, "ok");
+			old_status_state = of_property_status_ok(old_prop);
 	}
 
 	switch (action) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2 1/2] drivers/of: validate live-tree string properties before string use

[Rob Herring]

On Fri, Apr 17, 2026 at 08:36:00PM +0800, Pengpeng Hou wrote:
> `populate_properties()` stores live-tree property values as raw byte
> sequences plus a separate `length`. They are not globally guaranteed to
> be NUL-terminated.
> 
> `of_prop_next_string()` iterates string-list properties by walking raw
> bytes, `__of_node_is_type()` checks `device_type`,
> `__of_device_is_status()` checks `status`, and
> `of_alias_from_compatible()` reads the first `compatible` entry. These
> paths must validate that the relevant string fits within the property
> bounds before they hand it to C string helpers.
> 
> Validate these live-tree string properties within their declared bounds.
> In particular, make `of_prop_next_string()` reject malformed entries
> before returning them, use `of_property_match_string()` for
> `device_type`, and add unit coverage for malformed first and trailing
> string-list entries.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> Changes since v1:
> - use of_property_match_string() for device_type as suggested by
>   Rob Herring
> - rework of_prop_next_string() so the first returned string is validated
>   through the same bounded path
> - add of_unittest_property_string() coverage for malformed first and
>   trailing string-list entries

Did you even test this? The virt machine under QEMU doesn't even boot. 

[    0.000000] OF: reserved mem: Reserved memory: No reserved-memory node in the DT                                                                                                                                    

It hangs here.

[    0.000000] NUMA: Faking a node at [mem 0x0000000040000000-0x000000007fffffff]                                                                                                                                      

Re: [PATCH v2 1/2] drivers/of: validate live-tree string properties before string use

[Pengpeng Hou]

Hi Rob,

Thanks for catching this. You are right, I missed a lock recursion in
v2: __of_node_is_type() was changed to call of_property_match_string(),
but that helper takes devtree_lock. __of_node_is_type() is also used
from paths that already hold devtree_lock, including
of_find_node_by_type(), of_match_node(), and
of_find_matching_node_and_match(), so v2 can deadlock during early
boot.

I will send a v3 that keeps the device_type string bounded, but uses
__of_get_property() directly inside __of_node_is_type() so the helper
remains safe under devtree_lock.

Sorry about that.

Thanks,
Pengpeng

[PATCH v3 1/2] drivers/of: validate live-tree string properties before string use

[Pengpeng Hou]

`populate_properties()` stores live-tree property values as raw byte
sequences plus a separate `length`. They are not globally guaranteed to
be NUL-terminated.

`of_prop_next_string()` iterates string-list properties by walking raw
bytes, `__of_node_is_type()` checks `device_type`,
`__of_device_is_status()` checks `status`, and
`of_alias_from_compatible()` reads the first `compatible` entry. These
paths must validate that the relevant string fits within the property
bounds before they hand it to C string helpers.

Validate these live-tree string properties within their declared bounds.
In particular, make `of_prop_next_string()` reject malformed entries
before returning them, keep the `device_type` check inside the existing
no-lock helper path, and add unit coverage for malformed first and
trailing string-list entries.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v2:
- fix the QEMU virt boot hang reported by Rob Herring: v2 made
  __of_node_is_type() call of_property_match_string(), which takes
  devtree_lock while __of_node_is_type() is also called from paths that
  already hold devtree_lock, such as of_find_node_by_type(),
  of_match_node(), and of_find_matching_node_and_match()
- keep the device_type validation bounded, but use __of_get_property()
  directly so the helper remains safe under devtree_lock

Changes since v1:
- rework of_prop_next_string() so the first returned string is validated
  through the same bounded path
- add of_unittest_property_string() coverage for malformed first and
  trailing string-list entries

 drivers/of/base.c     | 43 ++++++++++++++++-----------
 drivers/of/property.c | 27 +++++++++++++----
 drivers/of/unittest.c | 32 ++++++++++++++++++++
 3 files changed, 80 insertions(+), 22 deletions(-)

diff --git a/drivers/of/base.c b/drivers/of/base.c
index 57420806c1a2..515e364bc4ac 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -82,9 +82,17 @@ EXPORT_SYMBOL(of_node_name_prefix);
 
 static bool __of_node_is_type(const struct device_node *np, const char *type)
 {
-	const char *match = __of_get_property(np, "device_type", NULL);
+	const char *match;
+	int len;
 
-	return np && match && type && !strcmp(match, type);
+	if (!np || !type)
+		return false;
+
+	match = __of_get_property(np, "device_type", &len);
+	if (!match || len <= 0 || strnlen(match, len) >= len)
+		return false;
+
+	return !strcmp(match, type);
 }
 
 #define EXCLUDED_DEFAULT_CELLS_PLATFORMS ( \
@@ -491,22 +499,22 @@ static bool __of_device_is_status(const struct device_node *device,
 		return false;
 
 	status = __of_get_property(device, "status", &statlen);
-	if (status == NULL)
+	if (!status || statlen <= 0)
+		return false;
+	if (strnlen(status, statlen) >= statlen)
 		return false;
 
-	if (statlen > 0) {
-		while (*strings) {
-			unsigned int len = strlen(*strings);
+	while (*strings) {
+		unsigned int len = strlen(*strings);
 
-			if ((*strings)[len - 1] == '-') {
-				if (!strncmp(status, *strings, len))
-					return true;
-			} else {
-				if (!strcmp(status, *strings))
-					return true;
-			}
-			strings++;
+		if ((*strings)[len - 1] == '-') {
+			if (!strncmp(status, *strings, len))
+				return true;
+		} else {
+			if (!strcmp(status, *strings))
+				return true;
 		}
+		strings++;
 	}
 
 	return false;
@@ -1217,10 +1225,11 @@ EXPORT_SYMBOL(of_find_matching_node_and_match);
 int of_alias_from_compatible(const struct device_node *node, char *alias, int len)
 {
 	const char *compatible, *p;
-	int cplen;
+	int ret;
 
-	compatible = of_get_property(node, "compatible", &cplen);
-	if (!compatible || strlen(compatible) > cplen)
+	ret = of_property_read_string_index(node, "compatible", 0,
+					    &compatible);
+	if (ret)
 		return -ENODEV;
 	p = strchr(compatible, ',');
 	strscpy(alias, p ? p + 1 : compatible, len);
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 50d95d512bf5..e97bfe357808 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -648,16 +648,31 @@ EXPORT_SYMBOL_GPL(of_prop_next_u32);
 
 const char *of_prop_next_string(const struct property *prop, const char *cur)
 {
-	const void *curv = cur;
+	const char *curv;
+	const char *end;
+	size_t len;
 
-	if (!prop)
+	if (!prop || !prop->value || !prop->length)
 		return NULL;
 
-	if (!cur)
-		return prop->value;
+	curv = cur ? cur : prop->value;
+	end = prop->value + prop->length;
 
-	curv += strlen(cur) + 1;
-	if (curv >= prop->value + prop->length)
+	if (curv < (const char *)prop->value || curv >= end)
+		return NULL;
+
+	if (cur) {
+		len = strnlen(curv, end - curv);
+		if (len >= end - curv)
+			return NULL;
+
+		curv += len + 1;
+		if (curv >= end)
+			return NULL;
+	}
+
+	len = strnlen(curv, end - curv);
+	if (len >= end - curv)
 		return NULL;
 
 	return curv;
diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index 29402958f11c..ee53363dfa84 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -713,6 +713,7 @@ static void __init of_unittest_parse_phandle_with_args_map(void)
 static void __init of_unittest_property_string(void)
 {
 	const char *strings[4];
+	const struct property *prop;
 	struct device_node *np;
 	int rc;
 
@@ -789,6 +790,37 @@ static void __init of_unittest_property_string(void)
 	strings[1] = NULL;
 	rc = of_property_read_string_array(np, "phandle-list-names", strings, 1);
 	unittest(rc == 1 && strings[1] == NULL, "Overwrote end of string array; rc=%i, str='%s'\n", rc, strings[1]);
+
+	/* of_prop_next_string() tests */
+	prop = of_find_property(np, "phandle-list-names", NULL);
+	strings[0] = of_prop_next_string(prop, NULL);
+	unittest(strings[0] && !strcmp(strings[0], "first"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(strings[0] && !strcmp(strings[0], "second"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(strings[0] && !strcmp(strings[0], "third"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(!strings[0],
+		 "of_prop_next_string() should return NULL at end of list\n");
+
+	prop = of_find_property(np, "unterminated-string", NULL);
+	strings[0] = of_prop_next_string(prop, NULL);
+	unittest(!strings[0],
+		 "of_prop_next_string() should reject unterminated first string\n");
+
+	prop = of_find_property(np, "unterminated-string-list", NULL);
+	strings[0] = of_prop_next_string(prop, NULL);
+	unittest(strings[0] && !strcmp(strings[0], "first"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(strings[0] && !strcmp(strings[0], "second"),
+		 "of_prop_next_string() failure; got '%s'\n", strings[0]);
+	strings[0] = of_prop_next_string(prop, strings[0]);
+	unittest(!strings[0],
+		 "of_prop_next_string() should reject unterminated trailing string\n");
 }
 
 #define propcmp(p1, p2) (((p1)->length == (p2)->length) && \
-- 
2.50.1 (Apple Git-155)

[PATCH v3 2/2] drivers/of: validate status properties in reconfig state changes

[Pengpeng Hou]

Live-tree reconfiguration properties also carry raw values plus explicit
lengths. `of_reconfig_get_state_change()` currently treats `status`
property values as NUL-terminated strings and feeds them straight into
`strcmp()`.

Factor the `"okay"` / `"ok"` check out into a helper that first verifies
that the property contains a bounded C string within `prop->length`.
Malformed `status` updates should be treated as not enabling the node.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
Changes since v2:
- no code change; carried with patch 1/2

Changes since v1:
- no change; carried in v2 because patch 1/2 was reworked

 drivers/of/dynamic.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index 1a06175def37..efee59ed371a 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -74,6 +74,20 @@ static const char *action_names[] = {
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
 
+static bool of_property_status_ok(const struct property *prop)
+{
+	const char *status;
+
+	if (!prop || !prop->value || prop->length <= 0)
+		return false;
+
+	status = prop->value;
+	if (strnlen(status, prop->length) >= prop->length)
+		return false;
+
+	return !strcmp(status, "okay") || !strcmp(status, "ok");
+}
+
 #define _do_print(func, prefix, action, node, prop, ...) ({	\
 	func("changeset: " prefix "%-15s %pOF%s%s\n",		\
 	     ##__VA_ARGS__, action_names[action], node,		\
@@ -135,11 +149,9 @@ int of_reconfig_get_state_change(unsigned long action, struct of_reconfig_data *
 
 	if (prop && !strcmp(prop->name, "status")) {
 		is_status = 1;
-		status_state = !strcmp(prop->value, "okay") ||
-			       !strcmp(prop->value, "ok");
+		status_state = of_property_status_ok(prop);
 		if (old_prop)
-			old_status_state = !strcmp(old_prop->value, "okay") ||
-					   !strcmp(old_prop->value, "ok");
+			old_status_state = of_property_status_ok(old_prop);
 	}
 
 	switch (action) {
-- 
2.50.1 (Apple Git-155)