Re: [PATCH v2 05/10] libfdt: overlay: Prepare fixup and fragment path helpers for reuse

[David Gibson]

On Wed, Oct 28, 2026 at 02:46:54PM -0700, Douglas Anderson wrote:
> Clean up `overlay_parse_fixup_entry()` and generalize
> `overlay_split_fragment_path()` in `libfdt/fdt_overlay.c` so both can be
> reused when updating `/__local_fixups__`:
> 
> - Change `poffset` from `int` to `uint32_t` in
>   `overlay_fixup_one_phandle()`, `overlay_parse_fixup_entry()`, and
>   `overlay_fixup_phandle()`, and parse into `unsigned long` before
>   validating the range.
> - Require `*(sep + 1)` to be an ASCII digit (`'0'..'9'`) and bound
>   `poffset` to `INT32_MAX - sizeof(uint32_t)` so negative strings (such
>   as `"-4"`), leading whitespace, or large values cannot silently

Huh, I'd assumed that strtoul() wouldn't accept negative strings, but
according to BUGS in strtoul(1) that might not be the case (at least
for Linux+glibc).  Good catch.

>   truncate or wrap around in `fdt_setprop_inplace_namelen_partial()`.
> - Drop the redundant `*sep != ':'` checks after `memchr(..., ':', ...)`.
> - Generalize `overlay_split_fragment_path()` to accept a length-bounded
>   `path` slice that is not necessarily NUL-terminated (using `memchr()`
>   instead of `strchr()`, and keeping the NUL-terminated property check
>   in `overlay_symbol_update()`) so it can also split `:`-delimited paths
>   from `/__fixups__`.

This may need some minor rebasing for the suggestions on the previous
patch, plus some additional minor refinements suggested below.

> 
> Assisted-by: Gemini:gemini-pro-prerelease
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> 
> Changes in v2:
> - New patch tightening fixup offset parsing and generalizing
>   overlay_split_fragment_path() for non-NUL-terminated paths in
>   libfdt/fdt_overlay.c
> 
>  libfdt/fdt_overlay.c | 46 +++++++++++++++++++++++---------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index f9d1a555ed5d..eb87b356145f 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -330,7 +330,7 @@ static int overlay_update_local_references(void *fdto, uint32_t delta)
>  static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
>  				     const char *path, uint32_t path_len,
>  				     const char *name, uint32_t name_len,
> -				     int poffset, uint32_t phandle)
> +				     uint32_t poffset, uint32_t phandle)
>  {
>  	fdt32_t phandle_prop;
>  	int fixup_off;
> @@ -368,7 +368,7 @@ static int overlay_fixup_one_phandle(void *fdto, int symbols_off,
>  static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
>  				     const char **pathp, uint32_t *path_lenp,
>  				     const char **namep, uint32_t *name_lenp,
> -				     int *poffsetp)
> +				     uint32_t *poffsetp)
>  {
>  	const char *path, *name, *fixup_end;
>  	const char *fixup_str = *valuep;
> @@ -376,7 +376,7 @@ static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
>  	uint32_t fixup_len;
>  	const char *sep;
>  	char *endptr;
> -	int poffset;
> +	unsigned long poffset;
>  
>  	fixup_end = memchr(*valuep, '\0', *lenp);
>  	if (!fixup_end)
> @@ -388,7 +388,7 @@ static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
>  
>  	path = fixup_str;
>  	sep = memchr(fixup_str, ':', fixup_len);
> -	if (!sep || *sep != ':')
> +	if (!sep)
>  		return -FDT_ERR_BADOVERLAY;
>  
>  	path_len = sep - path;
> @@ -398,15 +398,17 @@ static int overlay_parse_fixup_entry(const char **valuep, int *lenp,
>  	fixup_len -= path_len + 1;
>  	name = sep + 1;
>  	sep = memchr(name, ':', fixup_len);
> -	if (!sep || *sep != ':')
> +	if (!sep)
>  		return -FDT_ERR_BADOVERLAY;
>  
>  	name_len = sep - name;
>  	if (!name_len)
>  		return -FDT_ERR_BADOVERLAY;
>  
> +	if ((*(sep + 1) < '0') || (*(sep + 1) > '9'))
> +		return -FDT_ERR_BADOVERLAY;

Putting the rationale for this in a comment here would be more useful
than just in the commit message.

>  	poffset = strtoul(sep + 1, &endptr, 10);
> -	if ((*endptr != '\0') || (endptr <= (sep + 1)))
> +	if ((*endptr != '\0') || (poffset > (INT32_MAX - sizeof(uint32_t))))
>  		return -FDT_ERR_BADOVERLAY;
>  
>  	*pathp = path;
> @@ -470,8 +472,8 @@ static int overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>  
>  	do {
>  		const char *path, *name;
> -		uint32_t path_len, name_len;
> -		int poffset, ret;
> +		uint32_t path_len, name_len, poffset;
> +		int ret;
>  
>  		ret = overlay_parse_fixup_entry(&value, &len, &path, &path_len,
>  						&name, &name_len, &poffset);
> @@ -925,8 +927,8 @@ static int get_path_len(const void *fdt, int nodeoffset)
>  
>  /**
>   * overlay_split_fragment_path - Split "/<frag>/__overlay__[/<rel>]" into parts
> - * @path: Input NUL-terminated path property string
> - * @path_len: Length of @path in bytes (including trailing NUL)
> + * @path: Input path string (need not be NUL-terminated)
> + * @path_len: Length of @path in bytes (excluding any terminator)
>   * @frag_namep: Output pointer to the fragment name
>   * @frag_name_lenp: Output length of @frag_namep in bytes
>   * @rel_pathp: Output pointer to the relative subnode path after __overlay__
> @@ -935,7 +937,7 @@ static int get_path_len(const void *fdt, int nodeoffset)
>   *
>   * returns:
>   *      0 on success
> - *      -FDT_ERR_BADVALUE if @path is not a valid NUL-terminated '/' path
> + *      -FDT_ERR_BADVALUE if @path does not start with '/'
>   */
>  static int overlay_split_fragment_path(const char *path, int path_len,
>  				       const char **frag_namep,
> @@ -951,18 +953,14 @@ static int overlay_split_fragment_path(const char *path, int path_len,
>  
>  	*is_overlayp = false;
>  
> -	/* verify it's a string property (terminated by a single \0) */
> -	if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1])
> -		return -FDT_ERR_BADVALUE;
> -
>  	/* keep end marker to avoid strlen() */
>  	e = path + path_len;
>  
> -	if (*path != '/')
> +	if (path_len < 1 || *path != '/')
>  		return -FDT_ERR_BADVALUE;
>  
>  	/* get fragment name first */
> -	s = strchr(path + 1, '/');
> +	s = memchr(path + 1, '/', path_len - 1);
>  	if (!s) {
>  		/* Symbol refers to something that won't end
>  		 * up in the target tree */
> @@ -972,13 +970,13 @@ static int overlay_split_fragment_path(const char *path, int path_len,
>  	frag_name = path + 1;
>  	frag_name_len = s - path - 1;
>  
> -	/* verify format; safe since "s" lies in \0 terminated prop */
> +	/* verify format */
>  	len = sizeof("/__overlay__/") - 1;
> -	if ((e - s) > len && (memcmp(s, "/__overlay__/", len) == 0)) {
> +	if ((e - s) >= len && (memcmp(s, "/__overlay__/", len) == 0)) {
>  		/* /<fragment-name>/__overlay__/<relative-subnode-path> */
>  		rel_path = s + len;
> -		rel_path_len = e - rel_path - 1;
> -	} else if ((e - s) == len
> +		rel_path_len = e - rel_path;
> +	} else if ((e - s) == (len - 1)
>  		   && (memcmp(s, "/__overlay__", len - 1) == 0)) {
>  		/* /<fragment-name>/__overlay__ */
>  		rel_path = "";
> @@ -1049,7 +1047,11 @@ static int overlay_symbol_update(void *fdt, void *fdto)
>  		if (!path)
>  			return path_len;
>  
> -		ret = overlay_split_fragment_path(path, path_len,
> +		/* verify it's a string property (terminated by a single \0) */
> +		if (path_len < 1 || memchr(path, '\0', path_len) != &path[path_len - 1])
> +			return -FDT_ERR_BADVALUE;
> +
> +		ret = overlay_split_fragment_path(path, path_len - 1,
>  						  &frag_name, &frag_name_len,
>  						  &rel_path, &rel_path_len,
>  						  &is_overlay);
> -- 
> 2.56.0.313.g2479322e68-goog
> 
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

Attachment: signature.asc
Description: PGP signature