Re: [PATCH] fdtoverlay: Prevent overlays from modifying phandle properties

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]



On Sat, 19 Dec 2020 at 17:28, Paul Barker <pbarker@xxxxxxxxxxxx> wrote:
>
> When applying an overlay fragment, we should take care not to overwrite
> an existing phandle property of the target node as this could break
> references to the target node elsewhere in the base dtb.
>
> In addition to potentially breaking references within the resulting fdt,
> if the overlay is built with symbols enabled (`-@` option to dtc) then
> fdtoverlay will be unable to merge the overlay with a base dtb file.
>
> A new test case is added to check how fdtoverlay handles this case.
> Attempting to apply this test overlay without the fix in this patch
> results in the following output:
>
>     input  = tests/overlay_base_ref.test.dtb
>     output = tests/overlay_overlay_ref.fdtoverlay.dtb
>     overlay[0] = tests/overlay_overlay_ref.test.dtb
>
>     Failed to apply 'tests/overlay_overlay_ref.test.dtb': FDT_ERR_NOTFOUND
>
> In this test case the __overlay__ node in question does not explicitly
> contain a phandle property in the dts file, the phandle is added during
> compilation as it is referenced by another node within the overlay dts.
>
> This failure occurs due to a sequence of events in the functions called
> by fdt_overlay_apply():
>
> 1) In overlay_fixup_phandles(), the target of the overlay fragment is
>    looked up and the target property is set to the phandle of the target
>    node.
>
> 2) In overlay_merge(), the target node is looked up by phandle via
>    overlay_get_target(). As the __overlay__ node in this test case
>    itself has a phandle property, the phandle of the target node is
>    modified.
>
> 3) In overlay_symbol_update(), the target node is again looked up by
>    phandle via overlay_get_target(). But this time the target node
>    cannot be found as its phandle property was modified.
>
> The fix for this issue is to skip modification of the phandle property
> of the target node in step (2) of the above sequence. If the target node
> doesn't already contain a phandle property, we can add one without risk.
>
> Signed-off-by: Paul Barker <pbarker@xxxxxxxxxxxx>
> ---
>  libfdt/fdt_overlay.c          |  2 ++
>  tests/overlay_base_ref.dts    | 19 +++++++++++++++++++
>  tests/overlay_overlay_ref.dts | 24 ++++++++++++++++++++++++
>  tests/run_tests.sh            |  5 +++++
>  4 files changed, 50 insertions(+)
>  create mode 100644 tests/overlay_base_ref.dts
>  create mode 100644 tests/overlay_overlay_ref.dts
>
> diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> index d217e79..b3c217a 100644
> --- a/libfdt/fdt_overlay.c
> +++ b/libfdt/fdt_overlay.c
> @@ -573,6 +573,8 @@ static int overlay_apply_node(void *fdt, int target,
>                 if (prop_len < 0)
>                         return prop_len;
>
> +               if (!strcmp(name, "phandle") && fdt_getprop(fdt, target, name, NULL))
> +                       continue;
>                 ret = fdt_setprop(fdt, target, name, prop, prop_len);
>                 if (ret)
>                         return ret;
> diff --git a/tests/overlay_base_ref.dts b/tests/overlay_base_ref.dts
> new file mode 100644
> index 0000000..1fc02a2
> --- /dev/null
> +++ b/tests/overlay_base_ref.dts
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +       test: test-node {
> +               test-int-property = <42>;
> +       };
> +
> +       test-refs {
> +               refs = <&test>;
> +       };
> +};
> diff --git a/tests/overlay_overlay_ref.dts b/tests/overlay_overlay_ref.dts
> new file mode 100644
> index 0000000..a45c95d
> --- /dev/null
> +++ b/tests/overlay_overlay_ref.dts
> @@ -0,0 +1,24 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + * Copyright (c) 2016 Konsulko Inc.
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +       fragment@0 {
> +               target = <&test>;
> +
> +               frag0: __overlay__ {
> +                       test-int-property = <43>;
> +               };
> +       };
> +
> +    test-ref {
> +        ref = <&frag0>;
> +    };
> +};
> diff --git a/tests/run_tests.sh b/tests/run_tests.sh
> index 294585b..a65b166 100755
> --- a/tests/run_tests.sh
> +++ b/tests/run_tests.sh
> @@ -329,6 +329,11 @@ dtc_overlay_tests () {
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__symbols__"
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__fixups__"
>      run_test check_path overlay_base_with_aliases.dtb not-exists "/__local_fixups__"
> +
> +    # Test taking a reference to an overlay fragment
> +    run_dtc_test -@ -I dts -O dtb -o overlay_base_ref.test.dtb "$SRCDIR/overlay_base_ref.dts"
> +    run_dtc_test -@ -I dts -O dtb -o overlay_overlay_ref.test.dtb "$SRCDIR/overlay_overlay_ref.dts"
> +    run_wrap_test $FDTOVERLAY -i overlay_base_ref.test.dtb overlay_overlay_ref.test.dtb -o overlay_overlay_ref.fdtoverlay.dtb
>  }
>
>  tree1_tests () {
> --
> 2.26.2
>

Hi folks,

I'm sending a ping on this as I haven't heard any feedback in 2 weeks.
Is there anything I can do to help move this forward? Or is there an
alternative approach I should take to solving this issue?

Thanks,

-- 
Paul Barker
Konsulko Group



[Index of Archives]     [Device Tree]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux