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@xxxxxxxxxx/
>
> > Co-developed-by: Mike McTernan <mikemcternan@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> > ---
> > 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
