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@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.
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:
signature.asc
Description: PGP signature
