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