[PATCH] common: relocate fdt_blob in global_data for FDTSRC_EMBED case

[Evgeny Bachinin]

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
> <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.
> > > > >
[...]
> > > > > +
> > > > > +     /*
> > > > > +      * 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 <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.
>

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-7840469f0084@salutedevices.com/T/#m7324a6dce4bc3804978073017617a4e9cf6843fa

-- 
Best Regards,
Evgeny Bachinin