Hello Simon, On Thu, Dec 05, 2024 at 10:57:36AM -0700, Simon Glass wrote: > Hi Evgeny, > > On Wed, 4 Dec 2024 at 12:57, Evgeny Bachinin > <[email protected]> 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 > > > <[email protected]> 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. > > > > > [...] > > > > > + > > > > > + /* > > > > > + * 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
Fixed in follow up patch series [1] > > > > > > > > + * 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 <[email protected]> > > > > > > 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. > Oh, sorry for misleading (I'm not about performance). Under "faster" I meant the speed of adaptation the approach with OF_EMBED both on Vendor and mainline U-Boots. We had to externally pass the dtb into build system via EXT_DTB (in Vendor U-Boot), but, unfortunately, the Vendor U-Boot (basing on 2015.01) can't use EXT_DTB without OF_EMBED=y. > > > > 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 > Thank you for idea. Have been done in the follow up patch-series [1] Links: [1] https://lore.kernel.org/u-boot/20241211-dtb_dt_embedded_within_fdtdec-v1-0-7840469f0...@salutedevices.com/T/#m7324a6dce4bc3804978073017617a4e9cf6843fa -- Best Regards, Evgeny Bachinin
