[PATCH] common: relocate fdt_blob in global_data for FDTSRC_EMBED case
Simon Glass
sjg at chromium.org
Thu Dec 5 18:57:36 CET 2024
Hi Evgeny,
On Wed, 4 Dec 2024 at 12:57, Evgeny Bachinin
<eabachinin at salutedevices.com> wrote:
>
> Hello Simon,
> please, see below
>
> On Tue, Dec 03, 2024 at 08:55:15AM -0700, Simon Glass wrote:
> > Hi Evgeny,
> >
> > On Fri, 29 Nov 2024 at 03:03, Evgeny Bachinin
> > <eabachinin at salutedevices.com> wrote:
> > >
> > > Hello, dear reviewers!
> > >
> > > It was my 1st attempt to use b4-tool and it did not list all relevant
> > > reviewers.
> > >
> > > So, let me manually CC relevant stakeholders/maintainers.
> > > Please, take a look on the fix of bug/vulnerability
> > >
> > > P.S. If you wish, advise me, please, whether I need to re-send with
> > > updated CC list or just publish v2.
> > >
> > > On Mon, Nov 25, 2024 at 12:15:07PM +0300, Evgeny Bachinin wrote:
> > > > Patch resolves two kind of bugs, one of which is vulnerability related
> > > > to KASLR.
> > > >
> > > > === Issue briefly ===
> > > >
> > > > Working with FDT (via non-relocated gd::fdt_blob) from inside bootm
> > > > command may lead to the reading the garbage instead of FDT nodes. And
> > > > this can result in various side-effects depending on DTS nodes, being
> > > > parsed during bootm.
> > > >
> > > > But below is my specific story how I faced with this issue due to
> > > > MESON_RNG probing failure.
> > > >
> > > > === Bugs description ===
> > > >
> > > > 1) Bug is revealed on:
> > > > * configuration below
> > > > * U-boot 2024.10 - f919c3a889f ("Prepare v2024.10")
> > > >
> > > > It seems, the following patch is a trigger:
> > > > ea955eea4f ("fdt: automatically add /chosen/kaslr-seed if DM_RNG is enabled")
> > > >
> > > > Generally, CONFIG_OF_EMBED=y & CONFIG_RNG_MESON=y are the most
> > > > valuable ones for reproducing the issue.
> > > > ```
> > > > CONFIG_ARCH_FIXUP_FDT_MEMORY=y
> > > > CONFIG_CMD_FDT=y
> > > > CONFIG_DEFAULT_FDT_FILE=""
> > > > CONFIG_FDT_64BIT=y
> > > > CONFIG_OF_BOARD_SETUP=y
> > > > CONFIG_OF_CONTROL=y
> > > > CONFIG_OF_EMBED=y
> > > > CONFIG_OF_LIBFDT_ASSUME_MASK=0x0
> > > > CONFIG_OF_LIBFDT_OVERLAY=y
> > > > CONFIG_OF_LIBFDT=y
> > > > CONFIG_OF_LIST="meson-axg-our-device-name"
> > > > CONFIG_OF_REAL=y
> > > > CONFIG_OF_TRANSLATE=y
> > > > CONFIG_SUPPORT_OF_CONTROL=y
> > > > CONFIG_SYS_FDT_PAD=0x3000
> > > > CONFIG_TOOLS_OF_LIBFDT=y
> > > >
> > > > CONFIG_DM_RNG=y
> > > > CONFIG_RNG_MESON=y
> > > > ```
> > > >
> > > > 2) Due to CONFIG_OF_EMBED, the DTS is embedded into U-boot ELF and
> > > > accessible via __dtb_dt_begin symbol.
> > > >
> > > > On early boot stage (board_f.c) the fdtdec_setup() is called only
> > > > once before U-boot's relocation into top of RAM. fdtdec_setup()
> > > > initializes gd::fdt_blob for FDTSRC_EMBED case:
> > > > ```
> > > > gd->fdt_blob = dtb_dt_embedded();
> > > > gd->fdt_src = FDTSRC_EMBED;
> > > > ```
> > > >
> > > > 3) Then reloc_fdt() is called in board_f.c
> > > >
> > > > But due to CONFIG_OF_EMBED=y the reloc_fdt() does not update
> > > > gd::fdt_blob value (strictly speaking, it is impossible for
> > > > CONFIG_OF_EMBED=y, because U-boot ELF has not been relocated yet
> > > > at this moment).
> > > >
> > > > As a result after relocation we get fdt_blob, pointing to DTS address
> > > > before relocation:
> > > > ```
> > > > # bdinfo
> > > > <...>
> > > > relocaddr = 0x000000000fedf000
> > > > reloc off = 0x000000000eedf000
> > > > <...>
> > > > fdt_blob = 0x010ce6c0 << points to __dtb_dt_begin before relocation
> > > > new_fdt = 0x0000000000000000 << empty erroneously
> > > > fdt_size = 0x0000000000000000 << zero erroneously
> > > > ```
> > > >
> > > > 4) During bootm command (according to our ITS-config file) the Linux
> > > > is loaded into 0x01080000 (which is very close to fdt_blob addr
> > > > 0x010ce6c0).
> > > > ```
> > > > ## Loading kernel from FIT Image at 04000000 ...
> > > > Trying 'kernel' kernel subimage
> > > > <...>
> > > > Load Address: 0x01080000
> > > > ```
> > > >
> > > > So Linux image overwrites the gd::fdt_blob memory location
> > > > in RAM (0x010ce6c0).
> > > >
> > > > 5) Issue:
> > > >
> > > > Hence any manipulation with DTS (say, via FDT API) inside
> > > > implementation of bootm command leads to accessing the fdt_blob area
> > > > with garbage, that can lead to two situations:
> > > >
> > > > 5.1) Abort.
> > > >
> > > > Call to fdt_off_dt_struct() from fdt_next_tag() :: fdt_offset_ptr()::
> > > > fdt_offset_ptr_() returns with garbage, that leads to tagp value
> > > > being out of RAM top addr (256 Mb in our board), causing the abort:
> > > > ```
> > > > Boot cmd: bootm 0x4000000#boot_evt1
> > > > bootm_run_states()
> > > > <...>
> > > > image_setup_libfdt()
> > > > fdt_chosen()
> > > > fdt_kaslrseed()
> > > > uclass_get_device()
> > > > uclass_get_device_tail()
> > > > device_probe()
> > > > device_of_to_plat()
> > > > meson_rng_of_to_plat()
> > > > clk_get_by_name_optional()
> > > > clk_get_by_name()
> > > > clk_get_by_name_nodev()
> > > > ofnode_stringlist_search()
> > > > fdt_stringlist_search()
> > > > fdt_getprop()
> > > > fdt_get_property_namelen_()
> > > > fdt_first_property_offset()
> > > > fdt_check_node_offset_()
> > > > fdt_next_tag():
> > > > ```
> > > > tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > > ```
> > > > fdt_next_tag() tagp:0x22890766
> > > > fdt_next_tag() ram_top:0x10000000 (tagp OUT of RAM)
> > > > "Synchronous Abort" handler, esr 0x96000010, far 0x22890766
> > > > elr: 000000000108be24 lr : 000000000108be24 (reloc)
> > > > elr: 000000000ff6fe24 lr : 000000000ff6fe24
> > > > x0 : 0000000000000041 x1 : 0000000000000000
> > > > x2 : 000000000ff3b57c x3 : 0000000000000012
> > > > x4 : 000000000ded2ad5 x5 : 0000000000000020
> > > > x6 : 00000000ffffffe8 x7 : 000000000ded2f40
> > > > x8 : 00000000ffffffd8 x9 : 000000000000000d
> > > > x10: 0000000000000006 x11: 000000000001869f
> > > > x12: 000000000fffffff x13: 000000000fffffff
> > > > x14: 0000000000000000 x15: 000000000ded2abb
> > > > x16: 000000000ff3b080 x17: 0000000000000001
> > > > x18: 000000000ded3dc0 x19: 0000000022890766
> > > > x20: 00000000010cb0f0 x21: 00000000000015e4
> > > > x22: 000000000ff8f4d8 x23: 000000000000000b
> > > > x24: 000000000ded2fbc x25: 000000000ffe2000
> > > > x22: 000000000ff8f4d8 x23: 000000000000000b
> > > > x24: 000000000ded2fbc x25: 000000000ffe2000
> > > > x26: 000000000ffe2000 x27: 000000000000000b
> > > > x28: 000000000ff9cf2d x29: 000000000ded2f40
> > > >
> > > > Code: aa1603e1 91197484 52801742 94004de8 (b9400276)
> > > > ```
> > > >
> > > > 5.2) Vulnerability situation "KASLR is disabled".
> > > >
> > > > Almost the same as in (5.1), but 2 situations happen (depending on
> > > > the value of garbage):
> > > > * call to fdt_offset_ptr_() :: fdt_off_dt_struct(fdt)
> > > > returns not so big garbage, leading to tagp, being inside RAM.
> > > > * or calculations of absoffset inside fdt_offset_ptr() leads to
> > > > failure of the one of if() conditions with NULL as retval.
> > > >
> > > > Result is fdt_next_tag() interprets the tagp as FDT_END. And we are
> > > > returning from our callstack via functions' error paths, leading to
> > > > "No RNG device" and "KASLR disabled due to lack of seed":
> > > > ```
> > > > fdt_kaslrseed()
> > > > uclass_get_device()
> > > > <...>
> > > > device_probe()
> > > > device_of_to_plat()
> > > > meson_rng_of_to_plat()
> > > > clk_get_by_name()
> > > > clk_get_by_name_nodev()
> > > > <...>
> > > > fdt_stringlist_search()
> > > > fdt_getprop()
> > > > fdt_get_property_namelen_()
> > > > fdt_first_property_offset()
> > > > fdt_check_node_offset_()
> > > > fdt_next_tag():
> > > > ```
> > > > tagp = fdt_offset_ptr(fdt, offset, FDT_TAGSIZE);
> > > > ```
> > > > fdt_next_tag() tagp:0000000001890677
> > > > fdt_next_tag() ram_top:0x10000000 (tagp is inside RAM)
> > > > uclass_get_device_tail():486 device_probe() ret:-22
> > > > No RNG device
> > > > Starting kernel ...
> > > >
> > > > [ 0.000000] Linux version 6.9.12
> > > > [ 0.000000] KASLR disabled due to lack of seed
> > > > ```
> > > >
> > > > Signed-off-by: Evgeny Bachinin <EABachinin at salutedevices.com>
> > > > ---
> > > > Tested:
> > > > * on board with Amlogic AXG SoC.
> > > > * on sandbox: make sure that certain code from patch is called (of
> > > > course, when CONFIG_OF_EMBED=y)
> > > > * U-boot CI: https://github.com/u-boot/u-boot/pull/698/checks
> > > > ---
> > > > common/board_r.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/common/board_r.c b/common/board_r.c
> > > > index 62228a723e12f8742437acae380e82f79903a8f5..88dc756b2a5e5a44f55f9b0fd012adc798a8afdb 100644
> > > > --- a/common/board_r.c
> > > > +++ b/common/board_r.c
> > > > @@ -152,6 +152,15 @@ static int initr_reloc_global_data(void)
> > > > */
> > > > gd->env_addr += gd->reloc_off;
> > > > #endif
> > > > +
> > > > + /*
> > > > + * For CONFIG_OF_EMBED case the FDT is embedded into ELF, available by
> > > > + * __dtb_dt_begin. After U-boot ELF self-relocation to RAM top address
> >
> > U-Boot
> >
> > > > + * it is worth to update fdt_blob in global_data
> > > > + */
> > > > + if (IS_ENABLED(CONFIG_OF_EMBED))
> > > > + gd->fdt_blob = dtb_dt_embedded();
> > > > +
> > > > #ifdef CONFIG_EFI_LOADER
> > > > /*
> > > > * On the ARM architecture gd is mapped to a fixed register (r9 or x18).
> > > >
> > > > ---
> > > > base-commit: cca05617a8f585f3a98a8fa82f75cc68a530d771
> > > > change-id: 20241119-of_embed_fdt_blob_garbage-ea729a8236e1
> > > >
> > > > Best regards,
> > > > --
> > > > Evgeny Bachinin <EABachinin at salutedevices.com>
> >
> > Note that OF_EMBED is a development feature[1], for debugging with
> > JTAG, etc.
>
>
> Yes, that's right.
>
> Due to the legacy, it's ok for us (for now) when dtb is inside U-Boot
> ELF. We are migrating from Amlogic vendor source code base into
> upstream one and chose more obvious and fast way (OF_EMBED), available
> and workable both in vendor's (old) U-Boot and in upstream one.
> One day, we will get rid of OF_EMBED, being fully on mainline
> sources...
I am surprised that OF_EMBED is faster. It really shouldn't make any difference.
>
> Nevertheless, option exists and it is not prohibited for usage. And as
> my patch reveals there were bugs, one of which led to KASLR disabled
> on real production board.
>
>
> > Which board is this?
>
> Our production device is based on Amlogic SoC A113X (AXG family) and
> is not public. The closest board is S400 [1].
>
>
> > Also, you should normally disable relocation (with gd->flags
> > GD_FLG_SKIP_RELOC) when using OF_EMBED, since the code ends up
> > elsewhere, which is a pain when debugging.
>
> Thank you for your advise! I'll take it into account.
>
>
> > This macro is for use by fdtgrep...perhaps I should have added an
> > underscore at the end to make that clear.
>
> Sorry, did not grasp your comment.
> What macro?
> P.S I tried to guess you talked about FDTSRC_EMBED or OF_EMBED, but
> anyway I can't relate them with fdtgrep by seeking in repo.
Oh I mean fdtdec
>
>
> > So please add a function in
> > fdtdec to handle this, calling it from initr_reloc_global_data(),
> > perhaps.
>
> Have I understood you right?
> You meant to move initialization of fdt_blob by dtb_dt_embedded() into
> a separate function something like this:
> ```
> diff --git a/common/board_r.c b/common/board_r.c
> index abe2395346..424954ea67 100644
> --- a/common/board_r.c
> +++ b/common/board_r.c
> @@ -159,7 +159,7 @@ static int initr_reloc_global_data(void)
> * it is worth to update fdt_blob in global_data
> */
> if (IS_ENABLED(CONFIG_OF_EMBED))
> - gd->fdt_blob = dtb_dt_embedded();
> + fdtdec_setup_embed()
>
> #ifdef CONFIG_EFI_LOADER
> /*
> diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> index 6865f78c70..484954a262 100644
> --- a/lib/fdtdec.c
> +++ b/lib/fdtdec.c
> @@ -1664,6 +1664,12 @@ static void setup_multi_dtb_fit(void)
> }
> }
>
> +void fdtdec_setup_embed(void)
> +{
> + gd->fdt_blob = dtb_dt_embedded();
> + gd->fdt_src = FDTSRC_EMBED;
> +}
> +
> int fdtdec_setup(void)
> {
> int ret = -ENOENT;
> @@ -1698,8 +1704,7 @@ int fdtdec_setup(void)
> gd->fdt_blob = fdt_find_separate();
> gd->fdt_src = FDTSRC_SEPARATE;
> } else { /* embed dtb in ELF file for testing / development */
> - gd->fdt_blob = dtb_dt_embedded();
> - gd->fdt_src = FDTSRC_EMBED;
> + fdtdec_setup_embed();
> }
> }
> ```
Yes that's right...it keeps the access to dtb_dt_embedded() within fdtdec
>
>
> [1] https://docs.u-boot.org/en/latest/board/amlogic/s400.html
>
Regards,
Simon
More information about the U-Boot
mailing list