Re: [PATCH] libfdt: fdt_get_alias_namelen: Validate aliases

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

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