[PATCH v9] ARM: boot: Validate start of physical memory against DTB

[Geert Uytterhoeven]

Hi Linus,

On Thu, Sep 3, 2020 at 10:49 PM Linus Walleij <linus.walleij at linaro.org> wrote:
> I am trying to understand this thing, it looks useful!

Thanks for your (initial ;-) comments!

> On Wed, Sep 2, 2020 at 5:36 PM Geert Uytterhoeven
> <geert+renesas at glider.be> wrote:
> > @@ -254,8 +254,56 @@ not_angel:
>
> This looks like it happens before CONFIG_ARM_ATAG_DTB_COMPAT
> meaning it will not use the DTB that is augmented with ATAGs,
> especially not ATAG_MEM (0x54410002) correct?
>
> That seems sad, because that would actually be useful to me.
>
> Can you see if it is possible to put this in after the ATAGs have
> been merged into the DTB?

Right, this is done before the DTB is augmented with ATAGs.
Hence if the memory node in DT is bogus, my validation code may
do the wrong thing, and return a bogus address, too :-(

> >  #ifdef CONFIG_AUTO_ZRELADDR
> > +#ifdef CONFIG_USE_OF
> >                 /*
> > -                * Find the start of physical memory.  As we are executing
> > +                * Find the start of physical memory.
> > +                * Try the DTB first, if available.
> > +                */
> > +               adr     r0, LC1
> > +               ldr     sp, [r0]        @ get stack location
> > +               add     sp, sp, r0      @ apply relocation
> > +
> > +#ifdef CONFIG_ARM_APPENDED_DTB
> > +               /*
> > +                * Look for an appended DTB. If found, use it and
> > +                * move stack away from it.
> > +                */
> > +               ldr     r6, [r0, #4]    @ get &_edata
> > +               add     r6, r6, r0      @ relocate it
> > +               ldmia   r6, {r0, r5}    @ get DTB signature and size
> > +#ifndef __ARMEB__
> > +               ldr     r1, =0xedfe0dd0 @ sig is 0xd00dfeed big endian
> > +               /* convert DTB size to little endian */
> > +               eor     r2, r5, r5, ror #16
> > +               bic     r2, r2, #0x00ff0000
> > +               mov     r5, r5, ror #8
> > +               eor     r5, r5, r2, lsr #8
> > +#else
> > +               ldr     r1, =0xd00dfeed
> > +#endif
>
> We now have two and even three copies of this code,
> sort of.

Indeed.

> Then at the end after deciding to use the appended
> device tree we get the DTB size *again* and moves
> the sp beyond the DTB permanently as we do not
> want to damage the DTB of course.
>
> To me it looks like the DTB size gets added to sp
> a second time? First it is bumped by your code,
> then when the appended DTB is detected and
> augmented further down, it gets bumped again
> for no reason here:
>
> /* relocate some pointers past the appended dtb */
> add    r6, r6, r5
> add    r10, r10, r5
> add    sp, sp, r5
> dtb_check_done:
>
> I don't know if I'm right, I may be confused...

Before that, it has started with a "fresh" stack pointer:

    restart:        adr     r0, LC1
                    ldr     sp, [r0]
                    ldr     r6, [r0, #4]
                    add     sp, sp, r0

So the addition is done only once (ignoring the "temporarily
relocate..." cakewalk, doing "add sp, sp, r5", and "sub sp, sp, r5"
later).

> It would be better if we could avoid copy/paste and
> merge the code in here so we first check if there is a
> DTB, else there is not much to do, and if there is, we
> get the size, move the sp and do both operations:
>
> 1. Check for ATAGs and augment the DTB
> 2. Check for memory size in the DTB
>
> This way the ATAG-augmented DTB can be used
> for checking for the memory start.
>
> I understand that you need the physical address
> before turning on the caches, so what I am thinking
> is if it is possible to move the check for appended
> DTB and ATAG augmentation business up before
> the cache enablement in a separate patch and then
> modify it from there. Then you could potentially
> merge these two things.
>
> Maybe there is something conceptually wrong with
> this that I have overlooked... :/

I agree there are plenty of opportunities to improve of head.S.
Unfortunately there are also plenty of opportunities to break someone's
boot process ;-(

Nicolas' patch to reshuffle the registers looks like a good first step...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds