[PATCH] libfdt: fdt_get_alias_namelen: Validate aliases

[PATCH] libfdt: fdt_get_alias_namelen: Validate aliases

[Pierre-Clément Tosi]

Ensure that the alias found is valid i.e.

    An alias value is a device path and is encoded as a string.
    The value represents the full path to a node, ...

This protects against a stack overflow (fdt_path_offset_namelen() calls
fdt_get_alias_namelen() then fdt_path_offset(alias), ...) when /aliases
has an empty property with an empty name.

Co-developed-by: Mike McTernan <mikemcternan@google.com>
Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 libfdt/fdt_ro.c   | 11 ++++++++++-
 tests/aliases.dts |  3 +++
 tests/get_alias.c | 12 +++++++++++-
 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
index c4c520c..bda5c0d 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 && memchr(alias, '\0', len) && *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..8820974 100644
--- a/tests/aliases.dts
+++ b/tests/aliases.dts
@@ -8,6 +8,9 @@
 		s1 = &sub1;
 		ss1 = &subsub1;
 		sss1 = &subsubsub1;
+		badpath = "wrong";
+		badpathlong = "wrong/with/parts";
+		empty = "";
 	};
 
 	sub1: subnode@1 {
diff --git a/tests/get_alias.c b/tests/get_alias.c
index fb2c38c..4f3f6fd 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,6 +43,9 @@ int main(int argc, char *argv[])
 	test_init(argc, argv);
 	fdt = load_blob_arg(argc, argv);
 
+	check_alias(fdt, NULL, "badpath");
+	check_alias(fdt, NULL, "badpathlong");
+	check_alias(fdt, NULL, "empty");
 	check_alias(fdt, "/subnode@1", "s1");
 	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
 	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
-- 
2.42.0.609.gbb76f46606-goog


-- 
Pierre

Re: [PATCH] libfdt: fdt_get_alias_namelen: Validate aliases

[David Gibson]

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

On Sat, Oct 07, 2023 at 12:07:10PM +0100, Pierre-Clément Tosi wrote:
> Ensure that the alias found is valid i.e.
> 
>     An alias value is a device path and is encoded as a string.
>     The value represents the full path to a node, ...

Where's that quote from?  That is typically the case, but I think that
at least back in the old OF days, aliases referring to other aliases
was allowed.  Even if it's not strictly correct in modern usage, I'd
prefer to allow that usage if it's not too difficult to accomplish.

> This protects against a stack overflow (fdt_path_offset_namelen() calls
> fdt_get_alias_namelen() then fdt_path_offset(alias), ...) when /aliases
> has an empty property with an empty name.

It would be helpful to spell out that bad situation a bit more
precisely.  AFAICT the way we discover an alias at the beginning of a
path, it would have to have at least one character, which means I'm
confused as to how something with an empty name gets in here.

> Co-developed-by: Mike McTernan <mikemcternan@google.com>

I've never seen a "Co-developed-by" tag before, I'd suggest just
putting that in the text.  However all developers of the patch should
sign off on it, so I'd expect an S-o-b from both people.

> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  libfdt/fdt_ro.c   | 11 ++++++++++-
>  tests/aliases.dts |  3 +++
>  tests/get_alias.c | 12 +++++++++++-
>  3 files changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index c4c520c..bda5c0d 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 && memchr(alias, '\0', len) && *alias == '/'))

The first three conditions here look good to me.  You could even check
alias[len - 1] == '\0' to avoid scanning the entire string - an alias
should be one single string.

I'd prefer not to enforce that the alias in an absolute path (the last
check), if we can avoid it, since, as above, I think historically
aliases to aliases has been an allowed thing.

> +		return NULL;

Pity we can't return a more specific error here.  Oh well.

> +
> +	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..8820974 100644
> --- a/tests/aliases.dts
> +++ b/tests/aliases.dts
> @@ -8,6 +8,9 @@
>  		s1 = &sub1;
>  		ss1 = &subsub1;
>  		sss1 = &subsubsub1;
> +		badpath = "wrong";
> +		badpathlong = "wrong/with/parts";

As well as exercising the absolute path test, which I'd prefer to
avoid, there doesn't seem to me that these two cases exercise anything
much different from each other.

> +		empty = "";

This one's a good test.  It would be good to test a non-terminated
string too (e.g. nonull = [626164];


>  	};
>  
>  	sub1: subnode@1 {
> diff --git a/tests/get_alias.c b/tests/get_alias.c
> index fb2c38c..4f3f6fd 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;

Not sure what this case is for - we never call this with both values
NULL, right?  If we did seems like we shouldn't silently ignore it.

> +	if (!aliaspath)
>  		FAIL("fdt_get_alias(%s) failed\n", alias);

Isn't aliaspath == NULL correct for some of these new test cases?
You're failing unconditionally here.

> +	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,6 +43,9 @@ int main(int argc, char *argv[])
>  	test_init(argc, argv);
>  	fdt = load_blob_arg(argc, argv);
>  
> +	check_alias(fdt, NULL, "badpath");
> +	check_alias(fdt, NULL, "badpathlong");
> +	check_alias(fdt, NULL, "empty");
>  	check_alias(fdt, "/subnode@1", "s1");
>  	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
>  	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");

-- 
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] libfdt: fdt_get_alias_namelen: Validate aliases

[Pierre-Clément Tosi]

Hi David,

On Sun, Oct 08, 2023 at 06:13:21PM +1100, David Gibson wrote:
> On Sat, Oct 07, 2023 at 12:07:10PM +0100, Pierre-Clément Tosi wrote:
> > Ensure that the alias found is valid i.e.
> > 
> >     An alias value is a device path and is encoded as a string.
> >     The value represents the full path to a node, ...
> 
> Where's that quote from?  That is typically the case, but I think that

See the DT specification (v0.4, 3.3 /aliases node):

    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.
    [...]
    An alias value is a device path and is encoded as a string.
    The value represents the full path to a node, [...]

> at least back in the old OF days, aliases referring to other aliases
> was allowed.  Even if it's not strictly correct in modern usage, I'd
> prefer to allow that usage if it's not too difficult to accomplish.

Compatibility with OF is great but checking that the result is a full path is
key to preventing recursion. Otherwise any DT with "circular" aliases can cause
the stack overflow I mention below. For example, calling

    fdt_path_offset(fdt, "loop")

on

    /{
        aliases {
            loop = "loop";
        };
    };

(I've added this as a test case in v2)

> 
> > This protects against a stack overflow (fdt_path_offset_namelen() calls
> > fdt_get_alias_namelen() then fdt_path_offset(alias), ...) when /aliases
> > has an empty property with an empty name.
> 
> It would be helpful to spell out that bad situation a bit more
> precisely.  AFAICT the way we discover an alias at the beginning of a
> path, it would have to have at least one character, which means I'm
> confused as to how something with an empty name gets in here.

I've rephrased the recursion in a more concrete way in v2.

AFAICT, nothing currently checks that the "alias has at least one character"
and merging [1] will only cover the special "circular" alias of the empty string
being aliased to itself.

[1]: https://lore.kernel.org/devicetree-compiler/20231006124839.z7auhc3mk37gxios@google.com/

> 
> > Co-developed-by: Mike McTernan <mikemcternan@google.com>
> 
> I've never seen a "Co-developed-by" tag before, I'd suggest just
> putting that in the text.  However all developers of the patch should
> sign off on it, so I'd expect an S-o-b from both people.
> 

Okay, I now have a link to the original fix in the Android Open Source Project.

> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > ---
> >  libfdt/fdt_ro.c   | 11 ++++++++++-
> >  tests/aliases.dts |  3 +++
> >  tests/get_alias.c | 12 +++++++++++-
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index c4c520c..bda5c0d 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 && memchr(alias, '\0', len) && *alias == '/'))
> 
> The first three conditions here look good to me.  You could even check
> alias[len - 1] == '\0' to avoid scanning the entire string - an alias
> should be one single string.
> 
> I'd prefer not to enforce that the alias in an absolute path (the last
> check), if we can avoid it, since, as above, I think historically
> aliases to aliases has been an allowed thing.

I've replaced the memchr() call in v2 but kept the absolute path check for the
reason mentioned above.

> 
> > +		return NULL;
> 
> Pity we can't return a more specific error here.  Oh well.

Want me to introduce fdt_find_alias{,_namelen}() wrapped as

    const char *fdt_get_alias_namelen(...)
    {
        const char *alias;
        int err = fdt_find_alias_namelen(fdt, name, namelen, &alias);
        return err ? NULL : alias;
    }

? :)

This would allow callers to handle FDT_ERR_NOTFOUND differently from other
(probably more serious) errors currently hidden behind a NULL return value and
would allow our check to be reported as FDT_ERR_BADVALUE.

> 
> > +
> > +	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..8820974 100644
> > --- a/tests/aliases.dts
> > +++ b/tests/aliases.dts
> > @@ -8,6 +8,9 @@
> >  		s1 = &sub1;
> >  		ss1 = &subsub1;
> >  		sss1 = &subsubsub1;
> > +		badpath = "wrong";
> > +		badpathlong = "wrong/with/parts";
> 
> As well as exercising the absolute path test, which I'd prefer to
> avoid, there doesn't seem to me that these two cases exercise anything
> much different from each other.

Yes, the idea was that libfdt could get confused by the presence of separators
and consider 'badpathlong' valid even if it isn't absolute, hopefully increasing
coverage. I have instead merged these cases into a single "relative" test case.

> 
> > +		empty = "";
> 
> This one's a good test.  It would be good to test a non-terminated
> string too (e.g. nonull = [626164];

Done; do you know of a way to express a property with an empty name in DTS? Or
would covering that use-case require a handcrafted DTB?

> 
> 
> >  	};
> >  
> >  	sub1: subnode@1 {
> > diff --git a/tests/get_alias.c b/tests/get_alias.c
> > index fb2c38c..4f3f6fd 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;
> 
> Not sure what this case is for - we never call this with both values
> NULL, right?  If we did seems like we shouldn't silently ignore it.

This returns early if fdt_get_alias() was expected to return NULL and did so.
Perhaps, are you confusing 'aliaspath' with 'alias'?

> 
> > +	if (!aliaspath)
> >  		FAIL("fdt_get_alias(%s) failed\n", alias);
> 
> Isn't aliaspath == NULL correct for some of these new test cases?

Only if 'path' was NULL; see above.

> You're failing unconditionally here.
> 
> > +	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,6 +43,9 @@ int main(int argc, char *argv[])
> >  	test_init(argc, argv);
> >  	fdt = load_blob_arg(argc, argv);
> >  
> > +	check_alias(fdt, NULL, "badpath");
> > +	check_alias(fdt, NULL, "badpathlong");
> > +	check_alias(fdt, NULL, "empty");
> >  	check_alias(fdt, "/subnode@1", "s1");
> >  	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
> >  	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
> 
> -- 
> 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] libfdt: fdt_get_alias_namelen: Validate aliases

[David Gibson]

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

On Mon, Oct 09, 2023 at 03:09:23PM +0100, Pierre-Clément Tosi wrote:
> Hi David,
> 
> On Sun, Oct 08, 2023 at 06:13:21PM +1100, David Gibson wrote:
> > On Sat, Oct 07, 2023 at 12:07:10PM +0100, Pierre-Clément Tosi wrote:
> > > Ensure that the alias found is valid i.e.
> > > 
> > >     An alias value is a device path and is encoded as a string.
> > >     The value represents the full path to a node, ...
> > 
> > Where's that quote from?  That is typically the case, but I think that
> 
> See the DT specification (v0.4, 3.3 /aliases node):
> 
>     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.
>     [...]
>     An alias value is a device path and is encoded as a string.
>     The value represents the full path to a node, [...]
> 
> > at least back in the old OF days, aliases referring to other aliases
> > was allowed.  Even if it's not strictly correct in modern usage, I'd
> > prefer to allow that usage if it's not too difficult to accomplish.
> 
> Compatibility with OF is great but checking that the result is a full path is
> key to preventing recursion.

Ah, that's a fair point.

> Otherwise any DT with "circular" aliases can cause
> the stack overflow I mention below. For example, calling

>     fdt_path_offset(fdt, "loop")
> 
> on
> 
>     /{
>         aliases {
>             loop = "loop";
>         };
>     };

Right.  I think I got thrown off by the unnecessarily specific
description of the loop occurring with an empty alias name, which as
you show above doesn't need to be part of the issue.

> (I've added this as a test case in v2)
> 
> > 
> > > This protects against a stack overflow (fdt_path_offset_namelen() calls
> > > fdt_get_alias_namelen() then fdt_path_offset(alias), ...) when /aliases
> > > has an empty property with an empty name.
> > 
> > It would be helpful to spell out that bad situation a bit more
> > precisely.  AFAICT the way we discover an alias at the beginning of a
> > path, it would have to have at least one character, which means I'm
> > confused as to how something with an empty name gets in here.
> 
> I've rephrased the recursion in a more concrete way in v2.
> 
> AFAICT, nothing currently checks that the "alias has at least one character"

Well.. not explicitly.  And, yes, there are the cases of zero or
negative length path addressed by [1].  Apart from those cases,
however, we only enter the alias case if path[0] != '/' and the alias
name is from the start of the path to the first '/' - therefore the
alias must have at least one character.

> and merging [1] will only cover the special "circular" alias of the empty string
> being aliased to itself.
> 
> [1]: https://lore.kernel.org/devicetree-compiler/20231006124839.z7auhc3mk37gxios@google.com/
> 
> > 
> > > Co-developed-by: Mike McTernan <mikemcternan@google.com>
> > 
> > I've never seen a "Co-developed-by" tag before, I'd suggest just
> > putting that in the text.  However all developers of the patch should
> > sign off on it, so I'd expect an S-o-b from both people.
> > 
> 
> Okay, I now have a link to the original fix in the Android Open Source Project.
> 
> > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > > ---
> > >  libfdt/fdt_ro.c   | 11 ++++++++++-
> > >  tests/aliases.dts |  3 +++
> > >  tests/get_alias.c | 12 +++++++++++-
> > >  3 files changed, 24 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > > index c4c520c..bda5c0d 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 && memchr(alias, '\0', len) && *alias == '/'))
> > 
> > The first three conditions here look good to me.  You could even check
> > alias[len - 1] == '\0' to avoid scanning the entire string - an alias
> > should be one single string.
> > 
> > I'd prefer not to enforce that the alias in an absolute path (the last
> > check), if we can avoid it, since, as above, I think historically
> > aliases to aliases has been an allowed thing.
> 
> I've replaced the memchr() call in v2 but kept the absolute path check for the
> reason mentioned above.
> 
> > 
> > > +		return NULL;
> > 
> > Pity we can't return a more specific error here.  Oh well.
> 
> Want me to introduce fdt_find_alias{,_namelen}() wrapped as
> 
>     const char *fdt_get_alias_namelen(...)
>     {
>         const char *alias;
>         int err = fdt_find_alias_namelen(fdt, name, namelen, &alias);
>         return err ? NULL : alias;
>     }
> 
> ? :)
> 
> This would allow callers to handle FDT_ERR_NOTFOUND differently from other
> (probably more serious) errors currently hidden behind a NULL return value and
> would allow our check to be reported as FDT_ERR_BADVALUE.

No.  It's a minor nit that we can't return an error, but I don't think
it's worth the conceptual complexity cost of adding an additional
entry point to allow for it.

> > 
> > > +
> > > +	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..8820974 100644
> > > --- a/tests/aliases.dts
> > > +++ b/tests/aliases.dts
> > > @@ -8,6 +8,9 @@
> > >  		s1 = &sub1;
> > >  		ss1 = &subsub1;
> > >  		sss1 = &subsubsub1;
> > > +		badpath = "wrong";
> > > +		badpathlong = "wrong/with/parts";
> > 
> > As well as exercising the absolute path test, which I'd prefer to
> > avoid, there doesn't seem to me that these two cases exercise anything
> > much different from each other.
> 
> Yes, the idea was that libfdt could get confused by the presence of separators
> and consider 'badpathlong' valid even if it isn't absolute, hopefully increasing
> coverage. I have instead merged these cases into a single "relative" test case.
> 
> > 
> > > +		empty = "";
> > 
> > This one's a good test.  It would be good to test a non-terminated
> > string too (e.g. nonull = [626164];
> 
> Done; do you know of a way to express a property with an empty name in DTS? Or
> would covering that use-case require a handcrafted DTB?

I don't believe it's possible to express that in dts.  At one point I
was considering an "escaped" syntax to allow construction of node or
property names breaking the usual rules (largely for the purposes of
decompiling bad dtbs), but it doesn't look like I ever implemented it.

I'd also consider it a bug if dtc emitted a dtb with an empty node or
property name (other than the root node, which has an empty name by
convention).  Although it would be acceptable for that to be a "check"
rather than syntactic level check, which means we could still emit
such a thing with the -f option, or if the relevant check were
explicitly disabled.

> > 
> > 
> > >  	};
> > >  
> > >  	sub1: subnode@1 {
> > > diff --git a/tests/get_alias.c b/tests/get_alias.c
> > > index fb2c38c..4f3f6fd 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;
> > 
> > Not sure what this case is for - we never call this with both values
> > NULL, right?  If we did seems like we shouldn't silently ignore it.
> 
> This returns early if fdt_get_alias() was expected to return NULL and did so.
> Perhaps, are you confusing 'aliaspath' with 'alias'?

Oh, yes, sorry.  I was misreading it as (!path && !alias).

> > 
> > > +	if (!aliaspath)
> > >  		FAIL("fdt_get_alias(%s) failed\n", alias);
> > 
> > Isn't aliaspath == NULL correct for some of these new test cases?
> 
> Only if 'path' was NULL; see above.
> 
> > You're failing unconditionally here.
> > 
> > > +	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,6 +43,9 @@ int main(int argc, char *argv[])
> > >  	test_init(argc, argv);
> > >  	fdt = load_blob_arg(argc, argv);
> > >  
> > > +	check_alias(fdt, NULL, "badpath");
> > > +	check_alias(fdt, NULL, "badpathlong");
> > > +	check_alias(fdt, NULL, "empty");
> > >  	check_alias(fdt, "/subnode@1", "s1");
> > >  	check_alias(fdt, "/subnode@1/subsubnode", "ss1");
> > >  	check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
> > 
> 
> 
> 

-- 
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] libfdt: fdt_get_alias_namelen: Validate aliases

[Mike McTernan]

On Sat, 7 Oct 2023 at 12:07, Pierre-Clément Tosi <ptosi@google.com> wrote:
>
> Ensure that the alias found is valid i.e.
>
>     An alias value is a device path and is encoded as a string.
>     The value represents the full path to a node, ...
>
> This protects against a stack overflow (fdt_path_offset_namelen() calls
> fdt_get_alias_namelen() then fdt_path_offset(alias), ...) when /aliases
> has an empty property with an empty name.
>
> Co-developed-by: Mike McTernan <mikemcternan@google.com>
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>

Acked-by: Mike McTernan <mikemcternan@google.com>

> ---
>  libfdt/fdt_ro.c   | 11 ++++++++++-
>  tests/aliases.dts |  3 +++
>  tests/get_alias.c | 12 +++++++++++-
>  3 files changed, 24 insertions(+), 2 deletions(-)
>
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index c4c520c..bda5c0d 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 && memchr(alias, '\0', len) && *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..8820974 100644
> --- a/tests/aliases.dts
> +++ b/tests/aliases.dts
> @@ -8,6 +8,9 @@
>                 s1 = &sub1;
>                 ss1 = &subsub1;
>                 sss1 = &subsubsub1;
> +               badpath = "wrong";
> +               badpathlong = "wrong/with/parts";
> +               empty = "";
>         };
>
>         sub1: subnode@1 {
> diff --git a/tests/get_alias.c b/tests/get_alias.c
> index fb2c38c..4f3f6fd 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,6 +43,9 @@ int main(int argc, char *argv[])
>         test_init(argc, argv);
>         fdt = load_blob_arg(argc, argv);
>
> +       check_alias(fdt, NULL, "badpath");
> +       check_alias(fdt, NULL, "badpathlong");
> +       check_alias(fdt, NULL, "empty");
>         check_alias(fdt, "/subnode@1", "s1");
>         check_alias(fdt, "/subnode@1/subsubnode", "ss1");
>         check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
> --
> 2.42.0.609.gbb76f46606-goog
>
>
> --
> Pierre

Re: [PATCH] libfdt: fdt_get_alias_namelen: Validate aliases

[David Gibson]

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

On Tue, Oct 10, 2023 at 03:18:04PM +0100, Mike McTernan wrote:
> On Sat, 7 Oct 2023 at 12:07, Pierre-Clément Tosi <ptosi@google.com> wrote:
> >
> > Ensure that the alias found is valid i.e.
> >
> >     An alias value is a device path and is encoded as a string.
> >     The value represents the full path to a node, ...
> >
> > This protects against a stack overflow (fdt_path_offset_namelen() calls
> > fdt_get_alias_namelen() then fdt_path_offset(alias), ...) when /aliases
> > has an empty property with an empty name.
> >
> > Co-developed-by: Mike McTernan <mikemcternan@google.com>
> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> 
> Acked-by: Mike McTernan <mikemcternan@google.com>

Thanks, I've added this to the patch on merge.

> 
> > ---
> >  libfdt/fdt_ro.c   | 11 ++++++++++-
> >  tests/aliases.dts |  3 +++
> >  tests/get_alias.c | 12 +++++++++++-
> >  3 files changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> > index c4c520c..bda5c0d 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 && memchr(alias, '\0', len) && *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..8820974 100644
> > --- a/tests/aliases.dts
> > +++ b/tests/aliases.dts
> > @@ -8,6 +8,9 @@
> >                 s1 = &sub1;
> >                 ss1 = &subsub1;
> >                 sss1 = &subsubsub1;
> > +               badpath = "wrong";
> > +               badpathlong = "wrong/with/parts";
> > +               empty = "";
> >         };
> >
> >         sub1: subnode@1 {
> > diff --git a/tests/get_alias.c b/tests/get_alias.c
> > index fb2c38c..4f3f6fd 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,6 +43,9 @@ int main(int argc, char *argv[])
> >         test_init(argc, argv);
> >         fdt = load_blob_arg(argc, argv);
> >
> > +       check_alias(fdt, NULL, "badpath");
> > +       check_alias(fdt, NULL, "badpathlong");
> > +       check_alias(fdt, NULL, "empty");
> >         check_alias(fdt, "/subnode@1", "s1");
> >         check_alias(fdt, "/subnode@1/subsubnode", "ss1");
> >         check_alias(fdt, "/subnode@1/subsubnode/subsubsubnode", "sss1");
> >
> >
> 

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