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

[Rob Herring <robh@kernel.org>]

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)
> 
>