[PATCH v2] libfdt: fdt_get_alias_namelen: Validate aliases

[PATCH v2] libfdt: fdt_get_alias_namelen: Validate aliases

[Pierre-Clément Tosi]

Ensure that the alias found matches the device tree specification v0.4:

    Each property of the /aliases node defines an alias. The property
    name specifies the alias name. The property value specifies the full
    path to a node in the devicetree.

This protects against a stack overflow caused by

    fdt_path_offset_namelen(fdt, path, namelen)

calling (if 'path' contains no '/')

    fdt_path_offset(fdt, fdt_get_alias_namelen(fdt, path, namelen))

leading to infinite recursion on DTs with "circular" aliases.

This fix was originally written by Mike McTernan for Android in [1].

[1]: https://android.googlesource.com/platform/external/dtc/+/9308e7f9772bd226fea9925b1fc4d53c127ed4d5

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>

---
v2
  - replace memchr('/') with check on last character
  - add test coverage of a self-referencing alias
  - drop redundant test case on alias to non-absolute path
  - reference the DT spec and AOSP patch in the commit message
  - rephrase the infinite recursion case in the commit message
---
 libfdt/fdt_ro.c   | 11 ++++++++++-
 tests/aliases.dts |  4 ++++
 tests/get_alias.c | 14 +++++++++++++-
 3 files changed, 27 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index c4c520c..39b7c68 100644
--- a/libfdt/fdt_ro.c
+++ b/libfdt/fdt_ro.c
@@ -537,7 +537,16 @@ static const void *fdt_path_getprop_namelen(const void *fdt, const char *path,
 const char *fdt_get_alias_namelen(const void *fdt,
 				  const char *name, int namelen)
 {
-	return fdt_path_getprop_namelen(fdt, "/aliases", name, namelen, NULL);
+	int len;
+	const char *alias;
+
+	alias = fdt_path_getprop_namelen(fdt, "/aliases", name, namelen, &len);
+
+	if (!can_assume(VALID_DTB) &&
+	    !(len > 0 && alias && alias[len - 1] == '\0' && *alias == '/'))
+		return NULL;
+
+	return alias;
 }
 
 const char *fdt_get_alias(const void *fdt, const char *name)
diff --git a/tests/aliases.dts b/tests/aliases.dts
index 853479a..b880176 100644
--- a/tests/aliases.dts
+++ b/tests/aliases.dts
@@ -5,6 +5,10 @@
 	#size-cells = <0>;
 
 	aliases {
+		empty = "";
+		loop = "loop";
+		nonull = [626164];
+		relative = "rel/at/ive";
 		s1 = &sub1;
 		ss1 = &subsub1;
 		sss1 = &subsubsub1;
diff --git a/tests/get_alias.c b/tests/get_alias.c
index fb2c38c..d2888d6 100644
--- a/tests/get_alias.c
+++ b/tests/get_alias.c
@@ -21,9 +21,16 @@ static void check_alias(void *fdt, const char *path, const char *alias)
 
 	aliaspath = fdt_get_alias(fdt, alias);
 
-	if (path && !aliaspath)
+	if (!path && !aliaspath)
+		return;
+
+	if (!aliaspath)
 		FAIL("fdt_get_alias(%s) failed\n", alias);
 
+	if (!path)
+		FAIL("fdt_get_alias(%s) returned %s instead of NULL",
+		     alias, aliaspath);
+
 	if (strcmp(aliaspath, path) != 0)
 		FAIL("fdt_get_alias(%s) returned %s instead of %s\n",
 		     alias, aliaspath, path);
@@ -36,9 +43,14 @@ int main(int argc, char *argv[])
 	test_init(argc, argv);
 	fdt = load_blob_arg(argc, argv);
 
+	check_alias(fdt, NULL, "empty");
+	check_alias(fdt, NULL, "nonull");
+	check_alias(fdt, NULL, "relative");
 	check_alias(fdt, "/subnode@1", "s1");
 	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
 	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
 
+	check_alias(fdt, NULL, "loop"); // Might trigger a stack overflow
+
 	PASS();
 }
-- 
2.42.0.609.gbb76f46606-goog


-- 
Pierre

Re: [PATCH v2] libfdt: fdt_get_alias_namelen: Validate aliases

[David Gibson]

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

On Mon, Oct 09, 2023 at 03:20:04PM +0100, Pierre-Clément Tosi wrote:
> Ensure that the alias found matches the device tree specification v0.4:
> 
>     Each property of the /aliases node defines an alias. The property
>     name specifies the alias name. The property value specifies the full
>     path to a node in the devicetree.
> 
> This protects against a stack overflow caused by
> 
>     fdt_path_offset_namelen(fdt, path, namelen)
> 
> calling (if 'path' contains no '/')

Uh.. this still seems confusing, or at least misleadingly specific.
Having a self-referential alias doesn't really have anything to do
with whether the path has any '/' or not.

>     fdt_path_offset(fdt, fdt_get_alias_namelen(fdt, path, namelen))
> 
> leading to infinite recursion on DTs with "circular" aliases.
> 
> This fix was originally written by Mike McTernan for Android in [1].

Urgh... I don't love the idea of merging something that doesn't have a
Signed-off from the original author.  I guess it's probably ok with
something this small.

> [1]: https://android.googlesource.com/platform/external/dtc/+/9308e7f9772bd226fea9925b1fc4d53c127ed4d5
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> 
> ---
> v2
>   - replace memchr('/') with check on last character
>   - add test coverage of a self-referencing alias
>   - drop redundant test case on alias to non-absolute path
>   - reference the DT spec and AOSP patch in the commit message
>   - rephrase the infinite recursion case in the commit message
> ---
>  libfdt/fdt_ro.c   | 11 ++++++++++-
>  tests/aliases.dts |  4 ++++
>  tests/get_alias.c | 14 +++++++++++++-
>  3 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index c4c520c..39b7c68 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -537,7 +537,16 @@ static const void *fdt_path_getprop_namelen(const void *fdt, const char *path,
>  const char *fdt_get_alias_namelen(const void *fdt,
>  				  const char *name, int namelen)
>  {
> -	return fdt_path_getprop_namelen(fdt, "/aliases", name, namelen, NULL);
> +	int len;
> +	const char *alias;
> +
> +	alias = fdt_path_getprop_namelen(fdt, "/aliases", name, namelen, &len);
> +
> +	if (!can_assume(VALID_DTB) &&
> +	    !(len > 0 && alias && alias[len - 1] == '\0' && *alias == '/'))

I'd be more confortable with the alias test before the len > 0  test.
It's probably fine either way: if !alias, then len should be an error
code < 0.  However, the point is len has a different interpretation
depending on whether alias is NULL or not-NULL, making (len > 0) a
slightly ambiguous statement if we haven't already established which
case we're in.

> +		return NULL;
> +
> +	return alias;
>  }
>  
>  const char *fdt_get_alias(const void *fdt, const char *name)
> diff --git a/tests/aliases.dts b/tests/aliases.dts
> index 853479a..b880176 100644
> --- a/tests/aliases.dts
> +++ b/tests/aliases.dts
> @@ -5,6 +5,10 @@
>  	#size-cells = <0>;
>  
>  	aliases {
> +		empty = "";
> +		loop = "loop";
> +		nonull = [626164];
> +		relative = "rel/at/ive";

Since you're only testing fdt_get_alias() here, rather than the full
path_offset(), this is probably ok, but there is a bit of ambuguity
here in what's wrong with this.  What you're testing here is that this
is disallowed as an alias-to-an-alias.  But if you were resolving this
with path_offset(), you'd expect NOTFOUND even if aliases-to-aliases
were allowed, both since the alias it's relative to doesn't exist, and
the rest of the path won't resolve anyway.

I think it would be clearer and more robust to use a case here where
the *only* thing wrong with the alias is that it involves another
alias.  e.g.

		relative = "s1/subsubnode"

That would be an alias resolving to the same thing as 'ss1', if it
were not for the alias-to-an-alias prohibition.

>  		s1 = &sub1;
>  		ss1 = &subsub1;
>  		sss1 = &subsubsub1;
> diff --git a/tests/get_alias.c b/tests/get_alias.c
> index fb2c38c..d2888d6 100644
> --- a/tests/get_alias.c
> +++ b/tests/get_alias.c
> @@ -21,9 +21,16 @@ static void check_alias(void *fdt, const char *path, const char *alias)
>  
>  	aliaspath = fdt_get_alias(fdt, alias);
>  
> -	if (path && !aliaspath)
> +	if (!path && !aliaspath)
> +		return;
> +
> +	if (!aliaspath)
>  		FAIL("fdt_get_alias(%s) failed\n", alias);
>  
> +	if (!path)
> +		FAIL("fdt_get_alias(%s) returned %s instead of NULL",
> +		     alias, aliaspath);
> +
>  	if (strcmp(aliaspath, path) != 0)
>  		FAIL("fdt_get_alias(%s) returned %s instead of %s\n",
>  		     alias, aliaspath, path);
> @@ -36,9 +43,14 @@ int main(int argc, char *argv[])
>  	test_init(argc, argv);
>  	fdt = load_blob_arg(argc, argv);
>  
> +	check_alias(fdt, NULL, "empty");
> +	check_alias(fdt, NULL, "nonull");
> +	check_alias(fdt, NULL, "relative");
>  	check_alias(fdt, "/subnode@1", "s1");
>  	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
>  	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
>  
> +	check_alias(fdt, NULL, "loop"); // Might trigger a stack overflow
> +
>  	PASS();
>  }

-- 
David Gibson			| 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 --]

Re: [PATCH v2] libfdt: fdt_get_alias_namelen: Validate aliases

[Pierre-Clément Tosi]

On Tue, Oct 10, 2023 at 03:56:19PM +1100, David Gibson wrote:
> On Mon, Oct 09, 2023 at 03:20:04PM +0100, Pierre-Clément Tosi wrote:
> > Ensure that the alias found matches the device tree specification v0.4:
> > 
> >     Each property of the /aliases node defines an alias. The property
> >     name specifies the alias name. The property value specifies the full
> >     path to a node in the devicetree.
> > 
> > This protects against a stack overflow caused by
> > 
> >     fdt_path_offset_namelen(fdt, path, namelen)
> > 
> > calling (if 'path' contains no '/')
> 
> Uh.. this still seems confusing, or at least misleadingly specific.
> Having a self-referential alias doesn't really have anything to do
> with whether the path has any '/' or not.

Because, even if fdt_path_offset() is called with a path containing one or more
'/', the recursion will result in a fdt_path_offset() call with a path that
doesn't have one, right? Good point, I've removed that condition from the commit
message in v3.

> 
> >     fdt_path_offset(fdt, fdt_get_alias_namelen(fdt, path, namelen))
> > 
> > leading to infinite recursion on DTs with "circular" aliases.
> > 
> > This fix was originally written by Mike McTernan for Android in [1].
> 
> Urgh... I don't love the idea of merging something that doesn't have a
> Signed-off from the original author.  I guess it's probably ok with
> something this small.
> 
> > [1]: https://android.googlesource.com/platform/external/dtc/+/9308e7f9772bd226fea9925b1fc4d53c127ed4d5
> > 
> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > 
> > ---
> > v2
> >   - replace memchr('/') with check on last character
> >   - add test coverage of a self-referencing alias
> >   - drop redundant test case on alias to non-absolute path
> >   - reference the DT spec and AOSP patch in the commit message
> >   - rephrase the infinite recursion case in the commit message
> > ---
> >  libfdt/fdt_ro.c   | 11 ++++++++++-
> >  tests/aliases.dts |  4 ++++
> >  tests/get_alias.c | 14 +++++++++++++-
> >  3 files changed, 27 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index c4c520c..39b7c68 100644
> > --- a/libfdt/fdt_ro.c
> > +++ b/libfdt/fdt_ro.c
> > @@ -537,7 +537,16 @@ static const void *fdt_path_getprop_namelen(const void *fdt, const char *path,
> >  const char *fdt_get_alias_namelen(const void *fdt,
> >  				  const char *name, int namelen)
> >  {
> > -	return fdt_path_getprop_namelen(fdt, "/aliases", name, namelen, NULL);
> > +	int len;
> > +	const char *alias;
> > +
> > +	alias = fdt_path_getprop_namelen(fdt, "/aliases", name, namelen, &len);
> > +
> > +	if (!can_assume(VALID_DTB) &&
> > +	    !(len > 0 && alias && alias[len - 1] == '\0' && *alias == '/'))
> 
> I'd be more confortable with the alias test before the len > 0  test.
> It's probably fine either way: if !alias, then len should be an error
> code < 0.  However, the point is len has a different interpretation
> depending on whether alias is NULL or not-NULL, making (len > 0) a
> slightly ambiguous statement if we haven't already established which
> case we're in.

Makes sense, done.

> 
> > +		return NULL;
> > +
> > +	return alias;
> >  }
> >  
> >  const char *fdt_get_alias(const void *fdt, const char *name)
> > diff --git a/tests/aliases.dts b/tests/aliases.dts
> > index 853479a..b880176 100644
> > --- a/tests/aliases.dts
> > +++ b/tests/aliases.dts
> > @@ -5,6 +5,10 @@
> >  	#size-cells = <0>;
> >  
> >  	aliases {
> > +		empty = "";
> > +		loop = "loop";
> > +		nonull = [626164];
> > +		relative = "rel/at/ive";
> 
> Since you're only testing fdt_get_alias() here, rather than the full
> path_offset(), this is probably ok, but there is a bit of ambuguity
> here in what's wrong with this.  What you're testing here is that this
> is disallowed as an alias-to-an-alias.  But if you were resolving this
> with path_offset(), you'd expect NOTFOUND even if aliases-to-aliases
> were allowed, both since the alias it's relative to doesn't exist, and
> the rest of the path won't resolve anyway.
> 
> I think it would be clearer and more robust to use a case here where
> the *only* thing wrong with the alias is that it involves another
> alias.  e.g.
> 
> 		relative = "s1/subsubnode"
> 
> That would be an alias resolving to the same thing as 'ss1', if it
> were not for the alias-to-an-alias prohibition.

Thanks, that's a much better test case.

> 
> >  		s1 = &sub1;
> >  		ss1 = &subsub1;
> >  		sss1 = &subsubsub1;
> > diff --git a/tests/get_alias.c b/tests/get_alias.c
> > index fb2c38c..d2888d6 100644
> > --- a/tests/get_alias.c
> > +++ b/tests/get_alias.c
> > @@ -21,9 +21,16 @@ static void check_alias(void *fdt, const char *path, const char *alias)
> >  
> >  	aliaspath = fdt_get_alias(fdt, alias);
> >  
> > -	if (path && !aliaspath)
> > +	if (!path && !aliaspath)
> > +		return;
> > +
> > +	if (!aliaspath)
> >  		FAIL("fdt_get_alias(%s) failed\n", alias);
> >  
> > +	if (!path)
> > +		FAIL("fdt_get_alias(%s) returned %s instead of NULL",
> > +		     alias, aliaspath);
> > +
> >  	if (strcmp(aliaspath, path) != 0)
> >  		FAIL("fdt_get_alias(%s) returned %s instead of %s\n",
> >  		     alias, aliaspath, path);
> > @@ -36,9 +43,14 @@ int main(int argc, char *argv[])
> >  	test_init(argc, argv);
> >  	fdt = load_blob_arg(argc, argv);
> >  
> > +	check_alias(fdt, NULL, "empty");
> > +	check_alias(fdt, NULL, "nonull");
> > +	check_alias(fdt, NULL, "relative");
> >  	check_alias(fdt, "/subnode@1", "s1");
> >  	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
> >  	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
> >  
> > +	check_alias(fdt, NULL, "loop"); // Might trigger a stack overflow
> > +
> >  	PASS();
> >  }
> 
> -- 
> David Gibson			| 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



-- 
Pierre

Re: [PATCH v2] libfdt: fdt_get_alias_namelen: Validate aliases

[David Gibson]

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

On Tue, Oct 10, 2023 at 10:26:43AM +0100, Pierre-Clément Tosi wrote:
> On Tue, Oct 10, 2023 at 03:56:19PM +1100, David Gibson wrote:
> > On Mon, Oct 09, 2023 at 03:20:04PM +0100, Pierre-Clément Tosi wrote:
> > > Ensure that the alias found matches the device tree specification v0.4:
> > > 
> > >     Each property of the /aliases node defines an alias. The property
> > >     name specifies the alias name. The property value specifies the full
> > >     path to a node in the devicetree.
> > > 
> > > This protects against a stack overflow caused by
> > > 
> > >     fdt_path_offset_namelen(fdt, path, namelen)
> > > 
> > > calling (if 'path' contains no '/')
> > 
> > Uh.. this still seems confusing, or at least misleadingly specific.
> > Having a self-referential alias doesn't really have anything to do
> > with whether the path has any '/' or not.
> 
> Because, even if fdt_path_offset() is called with a path containing one or more
> '/', the recursion will result in a fdt_path_offset() call with a path that
> doesn't have one, right?

Well, not necessarily, but you can get a loop even without that.  Most
trivially with:
	aliases {
		loop = "loop/some/path/or/other"
	};

As long as the first call to fdt_path_offset() has a '/' in it, so
will every subsequent one, but you'll still get infinite recursion
trying to resolve 'loop'.

-- 
David Gibson			| 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 --]