Re: [PATCH v2] libfdt: Fix fdt_get_path() reading before the start of the buffer

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4906 bytes --]

On Fri, Oct 30, 2026 at 05:06:02PM +0000, Ayan Kumar Halder wrote:
> Asking fdt_get_path() for an offset past the root node's FDT_END_NODE
> tag makes it read before the start of the caller's buffer. Such an
> offset is not a node offset, so this needs a bad nodeoffset.
> 
> fdt_get_path() trims a component off the buffer for each level the walk
> comes back up, comparing the components it has against the depth from
> fdt_next_node(). On leaving the root node, fdt_next_node() takes the
> depth negative and returns the offset just past that tag rather than an
> error, as commit ce4d9c0f ("libfdt: Rework fdt_next_node()") intended so
> that fdt_node_end_offset_() can find where a node ends. The loop only
> tests the offset, so it runs once more and tries to trim a component
> that is not there, scanning back past the start of the buffer.
> 
> Stop the walk once the depth goes negative, as
> fdt_subnode_offset_namelen() and fdt_node_end_offset_() already do, and
> reject the offset up front with fdt_check_node_offset_() where input is
> not assumed valid. Add a bad-offset case to tests/get_path.c; run it
> under valgrind (tests/run_tests.sh -m) to catch the read.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>

I agree with Simon's comments, otherwise LGTM.

> ---
> Changes in v2:
>  - Reworded the commit message: say FDT_END_NODE rather than "end tag",
>    state the problem first, and make clear it needs a bad nodeoffset
>    (Simon, David).
>  - Referenced commit ce4d9c0f ("libfdt: Rework fdt_next_node()") for why
>    fdt_next_node() returns that offset (David).
>  - Also reject the offset up front with fdt_check_node_offset_(), under
>    can_assume(VALID_INPUT) (Simon, David).
>  - Added a bad-offset case to tests/get_path.c (Simon, David).
> 
> v1: Message-ID <20261001121036.2910243-1-ayan.kumar.halder@amd.com>
> 
>  libfdt/fdt_ro.c  |  8 ++++++--
>  tests/get_path.c | 30 ++++++++++++++++++++++++++++++
>  2 files changed, 36 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 11f2e2e..555bba0 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -573,7 +573,7 @@ const char *fdt_get_symbol(const void *fdt, const char *name)
>  int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
>  {
>  	int pdepth = 0, p = 0;
> -	int offset, depth, namelen;
> +	int offset, depth, namelen, err;
>  	const char *name;
>  
>  	FDT_RO_PROBE(fdt);
> @@ -581,8 +581,12 @@ int fdt_get_path(const void *fdt, int nodeoffset, char *buf, int buflen)
>  	if (buflen < 2)
>  		return -FDT_ERR_NOSPACE;
>  
> +	if (!can_assume(VALID_INPUT)
> +	    && ((err = fdt_check_node_offset_(fdt, nodeoffset)) < 0))
> +		return err;
> +
>  	for (offset = 0, depth = 0;
> -	     (offset >= 0) && (offset <= nodeoffset);
> +	     (offset >= 0) && (depth >= 0) && (offset <= nodeoffset);
>  	     offset = fdt_next_node(fdt, offset, &depth)) {
>  		while (pdepth > depth) {
>  			do {
> diff --git a/tests/get_path.c b/tests/get_path.c
> index 7349898..1681747 100644
> --- a/tests/get_path.c
> +++ b/tests/get_path.c
> @@ -63,6 +63,34 @@ static void check_path(void *fdt, const char *path)
>  	check_path_buf(fdt, path, pathlen, 2);
>  }
>  
> +/*
> + * An offset past the root node's FDT_END_NODE tag is not a node offset,
> + * but fdt_next_node() still returns it rather than an error (see commit
> + * ce4d9c0f "libfdt: Rework fdt_next_node()"), so fdt_get_path() has to
> + * recognise it from the depth going negative.  Before it did, the path
> + * trimming scanned back past the start of the buffer for such an offset.
> + * Run under valgrind (run_tests.sh -m) to catch that read.
> + */
> +static void check_path_bad_offset(void *fdt)
> +{
> +	int offset, depth = 0, badoffset = 0;
> +	char buf[64];
> +	int len, i;
> +
> +	/* The offset fdt_next_node() reports once it has left the tree. */
> +	for (offset = 0; offset >= 0; offset = fdt_next_node(fdt, offset, &depth))
> +		badoffset = offset;
> +
> +	for (i = 0; i < 8; i++) {
> +		memset(buf, POISON, sizeof(buf));
> +
> +		len = fdt_get_path(fdt, badoffset + i, buf, sizeof(buf));
> +		if (len != -FDT_ERR_BADOFFSET && len != -FDT_ERR_BADSTRUCTURE)
> +			FAIL("fdt_get_path(%d) returns %d instead of an error",
> +			     badoffset + i, len);
> +	}
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	void *fdt;
> @@ -76,5 +104,7 @@ int main(int argc, char *argv[])
>  	check_path(fdt, "/subnode@1/subsubnode");
>  	check_path(fdt, "/subnode@2/subsubnode@0");
>  
> +	check_path_bad_offset(fdt);
> +
>  	PASS();
>  }
> -- 
> 2.25.1
> 
> 

-- 
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]